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

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: lib/ExecutionEngine/IntelJITEvents/IntelJITEventListener.cpp:145
+    auto SecOrErr = Sym.getSection();
+    if (!SecOrErr) {
+      // TODO: Actually report errors helpfully.
----------------
bwyma wrote:
> 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.
That dyrtld code uses several section methods - getSectionLoadAddress(), getAddress() - so it could not work in case section missed. 

This piece of code does not use any Section interface. If Symbol does not refer a section then it could be and absolute symbol and Undef value would be OK here. 

If  absolute symbols could not be here - them checking for the error is OK.


================
Comment at: lib/ExecutionEngine/IntelJITEvents/IntelJITEventListener.cpp:150
+    }
+    object::section_iterator Sec = *SecOrErr;
+    uint64_t Index = Sec->getIndex();
----------------
bwyma wrote:
> 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?
> 
Undefined section means an absolute address(not local to the section) - so it is fine to compare only offsets for them. i.e. if both the location and the sequence correspond to an undefined section then they compared by offsets only.

If that address is local to the section - then it would match with only addresses from that section.


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

https://reviews.llvm.org/D59490