[PATCH] D76085: [DWARFLinker][dsymutil][NFC] add section index into address range.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D76085#1922182 <https://reviews.llvm.org/D76085#1922182>, @avl wrote:

> >> Memory objects:
> >> source debug info sections.
> >> NON-optimized debug info sections.
> > 
> > I'm not sure where this ^ happens in the scenario list you describe above.
>
> it happens at point 3. "lld resolves relocations and generates contents of output sections."
>  the result of this step would be non-optimized debug info section with resolved relocations(*). 
>  this result should be later passed to dsymutil if we speak about standard dsymutil scenario.


But in the dsymutil scenario, debug info is not linked into the executable - so there would be no "non-optimized linked debug info sections". So I'm still confused here.

>> And is this fundamentally necessary for the llvm-dsymutil functionality, 
>> but not needed for lld? Or is this an improvement that could be made 
>> (& tested) in llvm-dsymutil independently/prior to the lld work?
> 
> Yes. This is fundamentally necessary for dsymutil functionality.
>  dsymutil gets on the input - debug map(which maps symbol addresses in the object file and their linked addresses in binary)
>  and linked binary. So to preserve dsymutil behavior, we need to create the same input: debug map and linked binary.

Why does dsymutil need the linked binary? Isn't the debug map sufficient? & couldn't a similar mapping be produced part-way through linking inside lld?

> But for "debug info linking" in general, it is not necessary to have linked binary and debug map. 
>  All information available in lld before output sections are linked and generated. 
>  As soon as the linker built liveness information for the sections, the debug info could be optimized/linked.
>  As a result, when lld would resolve relocations and generate output - the size of generated sections would be smaller than in (*).
>  That possibility is the advantage of "linking debug info in lld" before "linking debug info in dsymutil".
> 
> This optimized behavior could not be tested with dsymutil, since dsymutil is not ready to work with a non-linked binary.
> 
> But, we could test this functionality from lld.
> 
>> Changes to code in the llvm/ repository should generally be tested that repository - not only by tests in another project like lld.
> 
> I agree. Having unit-tests for old dsymutil functionality and for this new functionality would be good.

Since dsymutil is in the llvm subproject it's not necessary to re-test its functionality at the unit level, though it can be handy to write unit tests for more targeted testing that might be hard to reach via end-to-end testing using llvm-dsymutil itself.
For the new functionality, it should be tested in the llvm subproject /somewhere/ when it's added - testing it only in lld is insufficient (because it would be easy for llvm developers to regress/break the functionality as they aren't required to test all other subprojects for their changes).

> I am OK to do them.
>  Though, if that is possible, I would prefer to make them later and integrate together with related functionality. 
>  This concrete change does not add new functionality.

What would add new functionality? If by new functionality you mean new user-visible functionality in some llvm installed executable (like lld or llvm-dsymutil) then, yes, one can write lots of untested code that adds no new functionality, until it does - when that new code is finally called from some production binary. The problem with only testing once that functionality surfaces into a binary is that we'll lose track of what all the new code is taht now needs testing - it's best to add testing as the behavior is implemented in the code.

> It only changes interface and this change is tested by existing dsymutil tests.
>  If that is not OK for current moment - I would start to work on creating set of unit-tests.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76085