In D62935#1537096, @steven_wu wrote:

Thanks. I was not sure if the new functions in LTOModule.h/.cpp were in the right file - but I don't see a better place to put them.

This is fine. Do you think you might want to query symbols from InputFile instead of LTOModule in the future?

In D62935#1537096, @steven_wu wrote:

We should look at either merging the implementations or providing an api function that provides access to the "llvm.linker.options" named metadata via the IRSymtab. It may not be possible to merge the implementations because in COFF and MACHO the representation of dependent libraries is a string of command line options; whereas, the ELF representation is an array of library strings. This is because for COFF and MACHO there is basically only one linker for each file format so the command line format is fixed. For ELF there are a greater variety of linkers, so I just pass the library strings through to the linker, without processing them into command line arguments, and it is deferred to the linker to interpret the library strings.

It would be good if the API is flexible to return either. Not against having a different API to return linker options but I think that is less clean.

Thanks for the response.

Great to see someone wanting to use this. During the RFC we decided not to set SHF_EXCLUDE on the .deplibs ELF section that is used to store the strings from these pragmas. This means that other linkers that don't support dependent libraries will not remove the sections and they will end up in the output. This shouldn't cause any functional issues; however, it might be noticed by developers and commented on. Perhaps we should change this decision? Also, have you considered the other option of having the build system invoke the clang with the --dependent-library=<specifier> switch. This is an alternative approach where modifying the source code is not appropriate, an example use is: https://reviews.llvm.org/D61742.

@grimar - this patch causes a regression in behaviour for references to script symbols. I think https://reviews.llvm.org/D61298 is a reasonable fix. Could you take a look?

Addressed review comments

I am keen to keep this moving.

No longer shortening "dependent libraries" to "deplibs" except for the .deplibs section (as this takes up bytes on disk).

LGTM! Thanks for waiting for me to confirm. Comments inline:

Sorry for the delay in replying. I was trying to understand if the upgrade path was important. Comments inline:

Hi Steven,

I have updated the diff to address review comments. I think we can continue to refine this patch in parallel with discussing the design further (I am not dismissing the concerns raised by @compnerd and @jyknight).

Addressed review comments.

In D60242#1456509, @pcc wrote:

In D60242#1456022, @bd1976llvm wrote:

Hi Peter,

I really like the concept. I had some nebulous concerns when I read the RFC; but, I didn't comment as I couldn't think of any alternatives to what you were proposing. What I am writing here is still not fully formed.. so feel free to ignore if it is unhelpful.

My main concern is that this is supporting a rare usecase and I don't really like the idea of adding complexity to the core tools for niche cases.

I feel like it would be better if this could be implemented via post-processing in some way; outside of the core tools.

Some aspect of this, like the linker being able to report all of the functions reachable from a given set of entry points, are of general utility though.

I wonder if there is any scope for implementing something different along the lines of:

1. Do an initial link.
2. Analyse the resulting elf and report all of the functions in each partition.
3. Do another link but this time provide an ordering file so that all of the functions for each partition are placed contiguously.
4. Extract the functions for each partition, apart form the first, into a partition file which also contains metadata specifying the address range.
5. Write zeros over all of the functions apart from those in the main partition and then compress the executable (saving the file space).
6. A loader extension patches in the missing functions for each partition when needed.

Hi Ben, thanks for the feedback. I shared a similar concern around complexity, but I don't see any reasonable alternatives to implementing this directly in the linker.

Your idea of using orderfiles is interesting, but I see at least two showstopper problems with it. The first is that it is incompatible with range extension thunks. Imagine that you have one main partition and two loadable partitions. An orderfile is used to sort the main partition's sections before partition 1's sections before partition 2's sections. We're going to split partition 1 and partition 2 into separate files, but the linker doesn't know that, so it may place a range extension thunk in the middle of partition 1's sections and have partition 2 call it. Now at runtime we will crash if partition 2 is loaded without partition 1 and it tries to call one of the range extension thunks in partition 1. We might be able to work around this issue somehow by telling the linker where the partition boundaries are, but at that point since the linker needs to know how you're going to split the file up anyway it might as well actually split it up for you.

The second is that replacing parts of the file with zeros and compressing it isn't always useful from a size perspective; in many cases it's the uncompressed size that matters because that's what takes up storage space on the device. One might consider uncompressing the code at program startup but that leads to other problems: it slows down startup and prevents the kernel from paging out the pages containing the uncompressed code.

I also see other problems that aren't showstoppers but are still significant:

• We're leaving some of the binary size gains on the table because we can't move exception-handling sections, dynamically relocatable sections (e.g. vtables) or symbol tables.
• Since we aren't splitting the symbol table the solution is more error prone since there's nothing stopping someone from dlsym'ing a symbol in an unloaded partition.
• This requires linking multiple times, which will slow down linking and hurt developer productivity, but the problem is made worse when (full) LTO is being used since we'll need to do LTO multiple times.

In D60274#1456450, @compnerd wrote:

The early check for ELF is the problem. I care very little about .linker-options itself as long as the ability to ensure that framework linkage is provided (i.e. -framework ..., and '-L ...` can be passed along as options to the linker).

In D60274#1456362, @compnerd wrote:

Am I mistaken in that this effectively prevents the use of options to handle frameworks? That really is a strong requirement and should be part of this patch.

Hi Peter,

Apologies that I didn't check the information on the original bug. The GDB behaviour does look reasonable, thanks.

<Trying again via phabricator rather than replying on the list.>

In D59553#1437167, @ruiu wrote:

In D59553#1436941, @dblaikie wrote:

In D59553#1436890, @probinson wrote:

Does this do the right thing for a 32-bit target? 64-bit is common, but not universal.

Re. the underlying DWARF issues:
I can't remember whether the DWARF committee has ever considered specifying a special value for "address does not exist" but I doubt it, because the DWARF spec states "If an entity has no associated machine code, none of these [address-related] attributes are specified." (DWARF v5 section 2.17, p.51.) So, the current position of the DWARF spec is that the linker is supposed to rewrite the DWARF, I guess.

I don't think that's the intent of the DWARF standard/committee - it's been pretty careful about designing features that work with existing linker behavior without requiring the linker to be DWARF aware in any way.

In D59553#1436899, @echristo wrote:

Likely the best way to solve this would be to put the debug information for the rest of it in the same section group as the function. In particular the subprogram die for the concrete function should be in a section group along with the function. This was part of the considerations with bringing other things back into the debug_info section in dwarf5 as well.

I'd worry about the overhead of making hunks of standalone DIEs for every function. (though, that said - we do actually use 4 bytes for both a CU-relative offset, and for a sec_offset, I think - so maybe it doesn't matter /that/ much (adding a unit header would be the main cost - and maybe that's significant enough to be problematic size-wise too)).

Currently linkers don't eliminate debug info for garbage-collected sections, so the debug info for exectuables/shared objects are larger than necessary, so you could save bytes of the final output by using more bytes for intermediate object files. That doesn't seem like a bad deal to me.

I strongly feel that we shouldn't implement this. This will tie groups and --gc-sections together when they are orthogonal features; e.g. COMDAT groups will *also* have to be considered by --gc-sections as a single unit. The ability to strip individual sections form COMDAT groups is an important feature. I will add similar comment to the abi list. I'm surprised that gold implements this behavior, back in 2017 gold and bfd-ld disagreed on this.

In D56516#1353258, @morehouse wrote:

@bd1976llvm: So do you still think this is a compiler bug? It sounds like ELF provides no guarantees about which COMDAT is kept. Which would mean we need to keep the weak symbols from going in a COMDAT.

In D56516#1352925, @morehouse wrote:

Does weak-vs-strong count towards that "api"? Because that's the only difference in the case I'm seeing...

In D56437#1351440, @serge-sans-paille wrote:

@bd1976llvm interesting. Unfortunately I failed to find this thread :-/

In D56516#1351953, @pcc wrote:

Can you create a minimal reproducer?

If objcopy is complaining about sh_link = 0 I suspect that there's some issue where a global referenced via !associated is being dropped.

The wording of the standard is unfortunate. This has been brought up and clarified (that COMDATs are the only legal groups currently) on the list previously.

There is a relevant discussion about non-COMDAT groups here: https://reviews.llvm.org/D56437.

I agree with Rui. My understanding is that groups in ELF were designed specifically to support C++ templates and inline functions which is why COMDAT groups are the only allowed kind.
Interestingly, I have been having a somewhat related discussion on this review: https://reviews.llvm.org/D53234.
Could the annobin tool use SHF_LINK_ORDER instead?

In D53234#1330686, @Aaron1011 wrote:

We need to resolve this point of disagreement. IMO that optimization is only invalid in regular compilation. I think that for LTO this is a valid optimization. LTO has optimizations that would normally be invalid - for example the Internalize pass.

I think the issue here is that an LLVM COMDAT has two different purposes:

1. De-duplicate section groups
2. Ensure that section group members are kept/removed as a unit
   
   In ELF files, you can have non-COMDAT section groups, which only accomplish purpose 2.

In D53234#1310254, @Aaron1011 wrote:

2. Retain the comdats in the output (this patch). Limits the optimization potential of LTO. !associated metadata sections don't need to be in a comdats.

I don't believe that this is the right way to think about this. This patch prevents LLVM from performing an invalid optimization - that is, selectively discarding global objects that the IR explicitly indicates should stay together.

Trying again... Phabricator didn't like the markup in my last comment attempt.

Hi Araon,

Hi Aron, I'm interested in reviewing this. Would you mind responding to my comments on https://reviews.llvm.org/D53234 first, as you have based the correctness of this on the correctness of that change?

In D53234#1266444, @Aaron1011 wrote:

So perhaps this code needs to handle comdat containing an object with associated metadata specially (e.g. could just include in the ExternalComdats set)?

My concern is that the current behavior would still be incorrect for other uses of COMDATS - or at the very least, quite counterintuitive.
If I specify a COMDAT for two global objects, I would expect one of the following things to happen:

1. Both objects appear in the final object, both still in the COMDAT.
2. Neither object appears in the final object, and the COMDAT section does not exist (both objects were optimized out).
   
   Having only one of the two objects be present, and having it lack a COMDAT, seems very unexpected to me - especially considering that the COMDAT section can be read using external tools (e.g. readelf).

Thanks for the comments everyone.

Added missing newline at end of test file

Thanks for the detailed reply Peter!

In D50688#1207394, @kschwarz wrote:

Thanks for the link! However, I'm not sure if this is really related. The issue there was that a non-alloc section should be placed into a program header explicitly via PHDRS commands, which might be a valid use case.

In D51027#1207356, @grimar wrote:

Also, note that we already support the following:

.stack address_expression (NOLOAD) :
{

but not the

.stack address_expression (INFO) :
{

yet. So I think it is worth to implement both for consistency and few users we have.

Hi George. Do we actually want this behaviour?

Hi. Sorry to be slow to comment on this.

This wasn't committed in time for the 7.0 release but I wonder if rL338703 + rL338709 should be put on the release branch? The current output is slightly embarrassing.

Hi Peter,

ping!

updated diff to address review comments

Having thought about this I have decided to try to simplify the help text as much as possible to aid maintenance.

In D48298#1136343, @grimar wrote:

In D48298#1136277, @bd1976llvm wrote:

I would propose adding the ability to retrieve a unique filtered list of arguments.

for (StringRef Name : Args.getAllUniqueArgValues(OPT_wrap))
  Symtab->addSymbolWrap<ELFT>(Name);

Relevant for other options: --defsym, --trace-symbol, --undefined etc..

I am not sure it is useful for other options we have, but it perhaps can be a nice refactoring for this particular case.

For example, I am not sure we should use it for --defsym, as in theory it can have different values: --defsym=foo=1 ... --defsym=foo=2.
It is OK to process all of them one by one like we do now. The same applies for --trace-symbol and --undefined - there is no issue to process them one by one.
I would not overcomplicate the current code for these cases until it is proven to be a useful change.

I would propose adding the ability to retrieve a unique filtered list of arguments.

Hi Rui,

Hi Rui! Thanks for looking at the patch.

Why are extra symbols *in the assembly* a problem? We normally don't print assembly

Hi Rafael,

Hi Rafael. The SN-Linker has the following rule for metadata sections:

LGTM., as long as this doesn't cause the number of symbols to double up?