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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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:
> > > 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 ?
> > I don't think it makes sense to open the file separately, search for a valid section index, then pass that into the API - compared to the API doing this for you. If you know the section you want, you pass it in, if not, you don't & you get the best effort (first instance, or perhaps APIs could be updated to return multiple responses when it's ambiguous).
> > 
> > I take it your idea was that this "open the file, search for some section index which covers the address, then use that section index when calling the underlying API" was meant as a stop-gap until some longer term solution where section indexes would always be passed in?
> > 
> > I don't think that's a future we'd really have - most of these uses/users aren't going to pass in section indexes (users of llvm-dwarfdump/objdump/symbolizeretc are unlikely to specify a section index on the command line too), so I think it makes sense to have the built-in functionality be what's currently grafted on top using getModuleSectionIndexForAddress - leave that as the built-in functionality as it was before and let users override/be specific if they want to be.
> >I take it your idea was that this "open the file, search for some section index which 
> >covers the address, then use that section index when calling the underlying API" was 
> >meant as a stop-gap until some longer term solution where section indexes would 
> >always be passed in?
> 
> yes. that`s right.
> 
> >I don't think that's a future we'd really have - most of these uses/users aren't 
> >going to pass in section indexes (users of llvm-dwarfdump/objdump
> >/symbolizeretc are unlikely to specify a section index on the command line too), so 
> >I think it makes sense to have the built-in functionality be what's currently grafted 
> >on top using getModuleSectionIndexForAddress - leave that as the built-in 
> >functionality as it was before and let users override/be specific if they want to be.
> 
> That is also thing which I am agree. i.e. users should have a  built-in functionality for setting proper SectionIndex. They should not do it manually. 
> 
> Thing which seems to me questionable is that - Is it OK to hide that builtin functionality under old symbolizer interface : 
> 
> symbolizeCode(..., object::SectionedAddress ModuleOffset)
> symbolizeInlinedCode(..., object::SectionedAddress ModuleOffset)
> symbolizeData(..., object::SectionedAddress ModuleOffset
> 
> i.e. if ModuleOffset.SectionIndex = UndefSection and if we will keep old behavior in this case. That looks questionable because different use cases could require different behaviour. But in this case some concrete behavior would already be done.
> 
> Instead we can specify interface so that it always receives correct unambitious information. i.e. these methods would always receive correct SectionIndex : 
> 
> symbolizeCode(..., object::SectionedAddress ModuleOffset)
> symbolizeInlinedCode(..., object::SectionedAddress ModuleOffset)
> symbolizeData(..., object::SectionedAddress ModuleOffset
> 
> so that SectionIndex either set into proper index of section, either set into Undef state and it means an absolute address.
> 
> That allows to implement several builtin behaviors separately. for example : we could implement this method for symbolizer : 
> 
> enumerateSections (uint64_t address, std::function<void(uint_64_t SectionIndex)> on_section); 
> 
> then llvm-dwarfdump/objdump/symbolizer would read an address from command line ask symbolizer for sections which specified address relates to and call symbolizeCode for the received info. Then this enumerateSections method would be builtin solution for them. 
> 
> If we will need another behavior(find first of, find last of/display error) then it would be possible to create another helper builtin routine(as part of symbolizer interface or as separate implementation). 
> 
> This patch follows that idea : it requires SectionIndex to be properly specified for symbolizeCode/InlinedCode/Data methods and provides helper getModuleSectionIndexForAddress() routine so that all existing tools would be able to call it and work the same way as before.  This getModuleSectionIndexForAddress routine assumed to be temporarily and should be replaced later with proper solution(s). Something like above enumerateSections() suggestion. 
> 
> So it looks like this solution allows API to help users and at the same time to implement several use cases. 
> 
> 
> 
I'm still not sure I follow the harm of having these APIs default to their old behavior - if the current solution is to have them all use this extra API to more awkwardly get the old behavior anyway.

"then llvm-dwarfdump/objdump/symbolizer would read an address from command line ask symbolizer for sections which specified address relates to and call symbolizeCode for the received info. Then this enumerateSections method would be builtin solution for them."

What's the new behavior you're describing here - how would dwarfdump/objdump/symbolizer behave differently if they enumerated the sections? What sort of user-facing behavior change are you picturing here?

I think the existing behavior's really not too problematic & fine to preserve (what do GNU tools do - eg: addr2line on an object file?) & API users can override the Undef section index to some specific section if/when they want to.


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

https://reviews.llvm.org/D58848