[PATCH] D58848: [DebugInfo] follow up for "add SectionedAddress to DebugInfo interfaces"

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:142
+uint64_t
+LLVMSymbolizer::getModuleSectionIndexForAddress(const std::string &ModuleName,
+                                                uint64_t Address) {
----------------
Any reason this is becoming a member function from a file-local function?


================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:146
+  std::pair<std::string, std::string> ParsedModuleName =
+      parseModuleName(ModuleName);
+
----------------
I hadn't spotted this code (or looked very closely at it, at least) in the original review.

I'm surprised/confused why the original change involved adding code that would look at input object file names at all. Why is this?

Hmm, I see looking at the llvm-symbolizer code that this logic looks up the section name and then passes that in along with the file name to the symbolizing routine.

But what's the point of that? It doesn't seem like it would change/improve the quality of the symbolizer's output compared to passing in a "I don't know what section" value, right?


================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:148-157
+  auto ObjectsOrErr = getOrCreateObjectPair(std::get<0>(ParsedModuleName),
+                                            std::get<1>(ParsedModuleName));
+  if (!ObjectsOrErr) {
+    // ignore error at this place.
+    consumeError(ObjectsOrErr.takeError());
+    Modules.erase(ModuleName);
+    ObjectPairForPathArch.erase(std::make_pair(std::get<0>(ParsedModuleName),
----------------
Lots more get<N> rather than first/second. But there's also enough uses here it might be nicer to name the two variables instead of accessing them by pair repeatedly.


================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:341
 
+std::pair<std::string, std::string>
+LLVMSymbolizer::parseModuleName(const std::string &ModuleName) const {
----------------
Perhaps this function should return StringRefs into the originally passed in string (perhaps the original input should be a StringRef too)?


================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:355
+
+  return std::pair<std::string, std::string>(BinaryName, ArchName);
+}
----------------
Would make_pair be simpler here?

Also potentially using std::move to avoid extra string copies:

  return std::make_pair(std::move(BinaryName), std::move(ArchName));


================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:443-444
+
+  auto ObjectsOrErr = getOrCreateObjectPair(std::get<0>(ParsedModuleName),
+                                            std::get<1>(ParsedModuleName));
   if (!ObjectsOrErr) {
----------------
rather than using the tuple accessor (get<N>) I think it's probably still more canonical to use ".first" and ".second" for pairs.


================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:448
     Modules.insert(
         std::make_pair(ModuleName, std::unique_ptr<SymbolizableModule>()));
     return ObjectsOrErr.takeError();
----------------
You could replace the "unique_ptr<T>()" with "nullptr" here.


================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:453
 
+  if (Objects.first == nullptr)
+    return static_cast<SymbolizableModule *>(nullptr);
----------------
I don't think much code in LLVM checks for explicit "== nullptr" - this would probably more canonically be written as "if (!Objects.first)"


================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:454
+  if (Objects.first == nullptr)
+    return static_cast<SymbolizableModule *>(nullptr);
+
----------------
Guess this cast is needed because of the conversion to Expected?

Is non-error, but null, a valid result from this function? That's a tricky thing, but could be the case.

I wonder if Expected<T*> should be constructible from nullptr_t to simplify this sort of case?


================
Comment at: llvm/lib/ExecutionEngine/IntelJITEvents/IntelJITEventListener.cpp:150-158
+
+    uint64_t SectionIndex = object::SectionedAddress::UndefSection;
+    auto SectOrErr = Sym.getSection();
+    if (SectOrErr) {
+      SectionIndex = SectOrErr.get()->getIndex();
+    }
+
----------------
Is this related to the rest of the change in this patch?

If not, it should probably go in a separate patch/commit.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58848/new/

https://reviews.llvm.org/D58848