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

[Brock Wyma via Phabricator via llvm-commits]

bwyma added inline comments.


================
Comment at: lib/ExecutionEngine/IntelJITEvents/IntelJITEventListener.cpp:145
+    auto SecOrErr = Sym.getSection();
+    if (!SecOrErr) {
+      // TODO: Actually report errors helpfully.
----------------
avl wrote:
> AFAIU there should not be error here. If Sym is an absolute symbol without section assigned then Undef value should be used as SectionIndex.
The revision I proposed here is based on the changes you made to the runtime dynamic loader here:
https://reviews.llvm.org/rL354972#change-fR8HfLHV0a69

Why are the changes you requested here not necessary for the dyrtld code?

If the getSection() routine cannot return an error code then the interface needs to be changed so it cannot return one. As long as it can return an error code then I think it should be checked.


================
Comment at: lib/ExecutionEngine/IntelJITEvents/IntelJITEventListener.cpp:150
+    }
+    object::section_iterator Sec = *SecOrErr;
+    uint64_t Index = Sec->getIndex();
----------------
avl wrote:
> It is neccessary to check iterator for Obj.section_end() here. Something like this : 
> 
> uint64_t SectionIndex = object::SectionedAddress::UndefSection;
> if (auto SectOrErr = Sym.getSection())
>   if (*SectOrErr != Obj.section_end())
>     SectionIndex = SectOrErr.get()->getIndex();
> 
> DILineInfoTable Lines = Context->getLineInfoForAddressRange(
>     {*AddrOrErr, SectionIndex}, Size, FileLineInfoKind::AbsoluteFilePath);
I'll modify this patch to validate the section iterator but I don't understand how you intend a query for a location in an undefined section to ever return a valid line info table.

Look at the code you added to containsPC():
https://reviews.llvm.org/rL354972#change-oZUqHTb07FJi

If both the location and the sequence correspond to an undefined section then how do you know it actually matches?  If you say a sequence from an undefined section matches a location from a undefined section then wouldn't you sometimes get the wrong answer?



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

https://reviews.llvm.org/D59490