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

Fri Mar 8 11:51:54 PST 2019

avl marked an inline comment as done.
avl added inline comments.

================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:146
+  std::pair<std::string, std::string> ParsedModuleName =
+      parseModuleName(ModuleName);
+
----------------
avl wrote:
> dblaikie wrote:
> > avl wrote:
> > > dblaikie wrote:
> > > > dblaikie wrote:
> > > > > 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?
> > > > I think this follows on from one of the comments I made in the original review - I don't know it's worth plumbing through the section index to any callers except the original one you had in mind (objdump disassembly) as all the other callers will need work to do anytihng other than what they do today (eg: the symbolizer could have support for multiple answers - the same address in multiple sections (in which case it'd need tests for that, etc)- but this bit of code isn't doing that)
> > > I think that my original description was not clear enough and there is some misunderstanding of new interface usage. Let`s clarify it now and decide what should it mean. 
> > > 
> > > I assume that SectionIndex should always be set in the correct value. Setting SectionIndex into Undef state could lead to incorrect work if relocations present. At least that is how it is currently done. That assumes that all callers of new interface need to do extra work to set SectionIndex properly. In some cases I already done that additional work, in some cases left it in Undef state(if it did not brake testing) and for some cases getModuleSectionIndexForAddress could be used for setting SectionIndex(that is the temporarily routine until proper code implemented). These two last cases assumed to be changed into proper implementation.
> > > 
> > > It differs from the logic - if SectionIndex presented then take it into account, if SectionIndex is not presented then handle it somehow. 
> > > 
> > > The rationale for that is that DWARFDebugLineTable looks to be improper place to decide how to handle "I don't know what section" case. Different use cases could assume different handling of that situation. So I decided that DWARFDebugLineTable should always receive full info and caller of this routine should decide how to handle this situation.
> > > 
> > > f.e. case "I don't know what section" could be handled : 
> > > 
> > > a) find correct section and set it.
> > > b) find several sections which match to local offset and use them.
> > > c) find last section which match to local offset and use it(behavior before fix).
> > > d) ignore that case. 
> > > f) display error ... something else...
> > > 
> > > Do you think we need to change it and  allow setting SectionIndex into Undef state and specify how  DWARFDebugLineTable would handle it ?
> > > 
> > > 
> > Ah.
> > 
> > But most (all?) executables only have one executable section, right?  That's why the way this worked before your change has worked OK for most/all users historically?
> > 
> > On that basis I think it makes sense to let the exsiting/previous APIs (witohut specifying a section index (ie: have the original functions, without any section index parameter - then overloaded with the new versions that allow the section index to be specified) - or perhaps having to specify an Undef one) to continue to work, while letting those callers who need to, specify it.
> right, most executables only have one executable section. That is why previous implementation worked correctly for them using only address as point locator. But for not-linked objects this is not right. Previous implementation did not work for them. i.e. if we allow previous implementation by setting SectionIndex = UndefSection then such a call would work for only linked executable. That means that DWARFDebugLine or LLVMSymbolizer would imply concrete state.  
> 
> following calls could be done for only linked executables : 
> 
> DWARFDebugLine.getFileLineInfoForAddress ( SectionedAddress.SectionIndex = UndefSection
> LLVMSymbolizer.symbolizeInlinedCode ( SectionedAddress.SectionIndex = UndefSection
> 
> it would be error to call them  for non-linked object file. That means that all places where we could expect non-linked object file needs to have something like this : 
> 
> if (linked executable) {
>      LLVMSymbolizer.symbolizeInlinedCode ( SectionedAddress.SectionIndex = UndefSection
> } else {
>      LLVMSymbolizer.symbolizeInlinedCode ( SectionedAddress.SectionIndex = getSectionIndex()
> 
> }
> 
> llvm-dwardump, llvm-objectdump, lld, llvm-symbolizer - could work not only with linked executable but with not-linked object files also. So it looks like most usages would expect both cases - linked executables and not-linked object files. Probably in that case stateless interface would be more convenient ? 
> 
> Current stateless interface does not assume whether it works for linked or not-linked object. It always receives correct SectionIndex and works equally for both types : 
> 
> LLVMSymbolizer.symbolizeInlinedCode ( SectionedAddress.SectionIndex = getSectionIndex()
> 
> The places where it becomes less convenient  is the code assumed to work with *only* linked-executable. In such places there would be neccessary to add code which would detect SectionIndex for new interface. But this looks to be quite natural task and written once it could be used later for free. I created such a routine - getModuleSectionIndexForAddress(though I agree this is quite naive implementation) .
> 
> The advantage of the new stateless interface is that it could not be used incorrectly. i.e. it would not be possible to use UndefSection for non-linked object files. 
> 
> Probably it would make sense to not keep old behavior without SectionIndex ?  
> 
> currently UndefSection used in following modules : 
> 
> 1. sanstats
> 2. sancov
> 3. llvm-xray
> 4. llvm-dwarfdump
> 
> 
David, do you think it makes sense to assume that SectionIndex should always be set in the correct value(not equal to undef) ? Or your preferred choice is to allow SectionIndex to be set into UndefState and assume that in that case  linked executables should be processed ?

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

https://reviews.llvm.org/D58848