[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:142
+uint64_t
+LLVMSymbolizer::getModuleSectionIndexForAddress(const std::string &ModuleName,
+                                                uint64_t Address) {
----------------
avl wrote:
> dblaikie wrote:
> > Any reason this is becoming a member function from a file-local function?
> the idea was:
> - to not copy ModuleName parsing code from Symbolizer. This code is neccessary to properly handle ModuleName on Darwin, since it contains  architecture postfix.
> - and another reason Is to use this function in any place where SectionIndex would be neccessary. Specifying Undef value is not enough usually, see later comment on the Undef SectionIndex usage.
Right, I understand the first bit - but that wouldn't change where this function should live.

The second bit - it'd be nice to move it potentially in a separate refactoring patch (and/or including it in the patch where a second caller is added to this function) to help isolate/separate independent changes.


================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:146
+  std::pair<std::string, std::string> ParsedModuleName =
+      parseModuleName(ModuleName);
+
----------------
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.


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

https://reviews.llvm.org/D58848