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

[bwyma]

Summary

Following change https://reviews.llvm.org/rL354972 the Intel JIT Listener would not report line table information because the section index didn't match.

There was a similar issue with the PerfJitEventListener here: https://reviews.llvm.org/D59189

This patch performs the section index lookup when building the object address used to query the line table information.

$ llvm-lit llvm/test/JitListener

• Testing: 2 tests, 2 threads --

PASS: LLVM :: JitListener/simple.ll (1 of 2)
PASS: LLVM :: JitListener/multiple.ll (2 of 2)
Testing Time: 0.36s

Expected Passes    : 2

[bwyma]

Remove the "TODO" item which is now done.

[avl]

Thanks for fixing this! Please check comments.

lib/ExecutionEngine/IntelJITEvents/IntelJITEventListener.cpp

145 ↗

(On Diff #191090)

AFAIU there should not be error here. If Sym is an absolute symbol without section assigned then Undef value should be used as SectionIndex.

150 ↗

(On Diff #191090)

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);

[bwyma]

lib/ExecutionEngine/IntelJITEvents/IntelJITEventListener.cpp

145 ↗

(On Diff #191090)

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.

150 ↗

(On Diff #191090)

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?

[bwyma]

Update the patch to validate the section iterator before using it to lookup the section index.

[avl]

lib/ExecutionEngine/IntelJITEvents/IntelJITEventListener.cpp

145 ↗

(On Diff #191090)

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.

150 ↗

(On Diff #191090)

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.

[avl]

LGTM. Thanks!

llvm/trunk/lib/ExecutionEngine/IntelJITEvents/IntelJITEventListener.cpp

for (const std::pair<SymbolRef, uint64_t> &P : computeSymbolSizes(*DebugObj)) {
     if (!AddrOrErr) {
       // TODO: Actually report errors helpfully.
       consumeError(AddrOrErr.takeError());
       continue;
     }
     uint64_t Addr = *AddrOrErr;
     uint64_t Size = P.second;
 
+    auto SecOrErr = Sym.getSection();
+    if (!SecOrErr) {
+      // TODO: Actually report errors helpfully.
+      consumeError(SecOrErr.takeError());
+      continue;
+    }
+    object::section_iterator Sec = *SecOrErr;
+    if (Sec == Obj.section_end())
+      continue;
+    uint64_t Index = Sec->getIndex();
+
     // Record this address in a local vector
     Functions.push_back((void*)Addr);
 
     // Build the function loaded notification message
     iJIT_Method_Load FunctionMessage =
       FunctionDescToIntelJITFormat(*Wrapper, Name->data(), Addr, Size);
-    // TODO: it is neccessary to set proper SectionIndex here.
-    // object::SectionedAddress::UndefSection works for only absolute addresses.
-    DILineInfoTable Lines = Context->getLineInfoForAddressRange({Addr, object::SectionedAddress::UndefSection}, Size);
+    DILineInfoTable Lines =
+      Context->getLineInfoForAddressRange({Addr, Index}, Size);
     DILineInfoTable::iterator Begin = Lines.begin();
     DILineInfoTable::iterator End = Lines.end();
     for (DILineInfoTable::iterator It = Begin; It != End; ++It) {
       LineInfo.push_back(
           DILineInfoToIntelJITFormat((uintptr_t)Addr, It->first, It->second));
     }
     if (LineInfo.size() == 0) {
       FunctionMessage.source_file_name = 0;