[PATCH] D58194: [DebugInfo] add SectionedAddress to DebugInfo interfaces.

[Alexey Lapshin via Phabricator via llvm-commits]

alexey.lapshin marked 23 inline comments as done.
alexey.lapshin added inline comments.


================
Comment at: llvm/include/llvm/Object/ObjectFile.h:139
+struct SectionedAddress {
+  SectionedAddress() : SectionIndex(UndefSection), Address(0) {}
+  SectionedAddress(uint64_t SI, uint64_t VMA)
----------------
dblaikie wrote:
> Maybe use non-static data member initializers rather than constructors? 
> (& then drop the other two ctors too - let this be a plain struct)
I changed it to non-static data member initializers. But it looks like I could not drop following constructors. Since if I add non-static data member initializers then it stopped to compile inplace initializers = {Addr, Index}. To compile inplace initializer - it require constructor SectionedAddress (Addr, Index). It also require default constructor for default instantiation.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:273
+    raw_ostream &OS, DWARFDataExtractor &rnglistData,
+    llvm::function_ref<Optional<llvm::SectionedAddress>(uint32_t)>
+        LookupPooledAddress,
----------------
dblaikie wrote:
> Is this llvm:: qualifier to disambiguate different things called SectionedAddress? Perhaps we could avoid having different things with that name and use only one of them - maybe move the one that's already in libDebugInfo into libObject? (perhaps as an incremental patch before the rest of this change)
moved SectionedAddress from libDebugInfo into linObject.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:973
 
-  DWARFCompileUnit *CU = getCompileUnitForAddress(Address);
+  DWARFCompileUnit *CU = getCompileUnitForAddress(Address.Address);
   if (!CU)
----------------
dblaikie wrote:
> Presumably this is going to have ambiguity problems if it doesn't have the section too? Which CU does it return if there are two CUs both with address zero in them (because they each describe functions in different sections), for instance?
Agreed. I left it as is to reduce size of the fix. Since It is not required for 40703 bug. I would put TODO comment here.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:977-978
+
+  getFunctionNameAndStartLineForAddress(CU, Address.Address, Spec.FNKind,
+                                        Result.FunctionName, Result.StartLine);
   if (Spec.FLIKind != FileLineInfoKind::None) {
----------------
dblaikie wrote:
> Similarly here
would put TODO comment here.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:890-893
+  // Search for relocatable addresses
+  uint32_t Result = lookupAddressMain(Address);
+
+  if (Result == UnknownRowIndex) {
----------------
dblaikie wrote:
> Under what conditions is this code needed? (it looks like this would only fail if the section index was incorrect? Which makes me think/feel like this is maybe working around a bug of some kind?)
That is how DebugLineInfo organized. If it has absolute addresses - it does not care which section they relate. So if caller ask for information for an AbsoluteAddress from section X then there would not be possible to find such entry. All absolute addresses put in Undef section(in DebugLineInfo table). But if Line Info table has Relocation information then it would create entry for specific SectionIndex. Earlier there was not this differrentiation because addresses were not marked with SectionIndex. Now addresses which have relocations marked with proper section index and absolute addresses marked with undex section index. Thus the algorithm is following :

find address in relocations for specified section. then if not found check it for Undef section which contains info about all absolute addresses. 

another alternative would be to set proper sectionIndex for absolute addresses while DebugLineInfo table create it`s map. Then there would not be neccessary to search twice, but there would be neccessary to create more complex map. I think this is just another way to implement the same.


================
Comment at: llvm/test/tools/llvm-objdump/Inputs/function-sections-line-numbers.cpp:1-25
+
+__attribute__((__noinline__)) 
+   int Pow(int number, int count);
+
+
+
+
----------------
dblaikie wrote:
> Seems like a rather long/complicated test case. Is it sufficient to compile this with -ffunction-sections:
> 
>   void f1() { }
>   void f2() { }
> 
> and then symbolize address 0?
I took above short test case and check how llvm-objdump shows line numbers. for llvm-symbolizer it would probably be necessary to extend command line to tell to which section specified address related.


================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:420-421
+  ModuleAddress.Address = Lookup;
+  ModuleAddress.SectionIndex =
+      getModuleSectionIndexForAddress(Obj, ModuleAddress.Address);
+  if (DILineInfo LineInfo = DICtx.getLineInfoForAddress(ModuleAddress))
----------------
dblaikie wrote:
> Perhaps this should be changed to produce more than one result? Maybe just leaving a FIXME (given that it's no worse with this change than it was before)
added TODO comment for this.


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

https://reviews.llvm.org/D58194