In D72197#1825494, @MaskRay wrote:

In D72197#1825390, @pcc wrote:

In D72197#1825247, @MaskRay wrote:

In D72197#1825012, @pcc wrote:

It looks like this change caused us to start rejecting:

.thumb
.thumb_func
.globl foo
.hidden foo
foo:
adr lr, foo + 1

with:

test.s:6:1: error: unsupported relocation on symbol
adr lr, foo + 1
^

This is exposed by this Android ART code: https://cs.android.com/android/platform/superproject/+/master:art/runtime/arch/arm/quick_entrypoints_arm.S;l=1826

I see a couple of possible fixes for this:

1. We could go back to the previous behaviour for global hidden symbols.
2. We could teach MC and LLD about the R_ARM_THM_ALU_PREL_11_0 relocation required to relocate this instruction. Arguably the ART code shouldn't be adding the 1 for Thumb here (because R_ARM_THM_ALU_PREL_11_0 adds the 1 itself), so ART would then need to be fixed.

foo is a hidden definition in the current object file. Can the ART code be changed to use a local symbol (adr lr, .Lfoo) instead?

It could, but I don't really see a good argument for local symbols having different behaviour to strong hidden symbols.

Not special casing hidden in the general case has the nice property that: the assembler behavior is not affected by the visibility. Only the binding can.

We can thus postpone decisions to the linker, and all the logic can be implemented on the linker side. Both a hidden defined symbol and a local symbol is considered non-preemptive, thus they have the same behavior at the linker stage.

The downside is that we increase object file sizes. Hidden symbols are not that common so the downside may be acceptable.

I rebased the patch and addressed the review comments. I also noticed that there was a problem with the patch when explicitly placing non-mergeable sections into one of the "default" mergeable sections (e.g. .debug_str). As these "default" mergeable sections were not being added to the map it wasn't possible to explicitly place symbols into these even if the symbols entsize was compatible (as they would be put into a uniqued section instead). This is not acceptable for these "default" sections since for some of these like e.g. .debug_str, current binutils consumers make the assumption that there can only be one of these sections. The fix for this is WIP and I haven't added LIT tests for it yet; but, I have put up the patch in WIP state to get comments on the approach taken.

In D72194#1821072, @smeenai wrote:

Is this worth merging into 10.0 if it lands after the branch?

Thanks, I'll address the points that you have raised.
I tried updating the assert and rebasing the patch but the assert is now firing in the It tests. I will have to investigate this tomorrow and update the patch then.

In D72194#1818766, @nickdesaulniers wrote:

This is a partial fix for

Is this still considered a partial fix? Seems pretty complete to me, otherwise what's still missing?

In D68101#1806475, @nickdesaulniers wrote:

In D68101#1806213, @rjmccall wrote:

In D68101#1806135, @nickdesaulniers wrote:

In D68101#1802280, @rjmccall wrote:

I laid out a series of three options before:

• Emit different object-file sections for different mergeability settings.
• Only mark an object-file section as mergeable if all the symbols in it would have the same mergeability settings.
• Stop implicitly using mergeability for "ordinary" sections (i.e. sections other than the string section).

Of the above, I really think #2 is the only complete solution.

Can you explain why you think #1 is not "complete"? All three seem to establish correctness; I can see how giving up on the optimization (#3) is not a great solution, but #1 doesn't have that problem and actually preserves it in more places. To be clear, this is just taking advantage of the ability to have multiple sections with the same name in an ELF object file; they will still be assembled into a single section in the linked image.

My understanding is that changing the flags on an MCSection retroactively is pretty counter to the architecture, so I'm not sure how we'd actually achieve #2.

Ah, ok, I reread https://reviews.llvm.org/D72194 and see that it's creating non-contiguous sections (option 1), with unique entity sizes. That should be fine. Dissenting opinion retracted. We should prefer https://reviews.llvm.org/D72194 with the commit message updated.

Updated the diff to address review comments and expand test coverage.

In D72197#1806225, @MaskRay wrote:

In D72197#1805438, @bd1976llvm wrote:

I wonder if is this really a bug or intentional. Clang and LLVM seem to ignore the possibility of symbol interposition (see: -fno-semantic-interposition). What is the LLVM policy for this?

Also, this code is missing a case for STB_GLOBAL and visibility != STV_DEFAULT. Actually, I have noticed before that this case is missing in various places in MC. As it stands this patch may cause a performance regression on platforms that use -fvisibility=hidden.

Visibility is irrelevant here. For a symbol defined in the same section as the instruction, the relocation record will reference a non-SECTION symbol instead of a STT_SECTION after this change. If the symbol is STV_HIDDEN, the linker will think the symbol is non-preemptible. So, no performance regression.

Thanks for the feedback. I'm sorry that I am being slow on this... v.busy at the moment. Will try to finish the patch in the next few days.

I wonder if is this really a bug or intentional. Clang and LLVM seem to ignore the possibility of symbol interposition (see: -fno-semantic-interposition). What is the LLVM policy for this?

In D68101#1802280, @rjmccall wrote:

The solution described in that comment is not acceptable. We are not going to tell users that they cannot assign symbols explicitly to sections because LLVM is unable to promise not to miscompile if they do. It is LLVM's responsibility to correctly compile valid code; enabling mergeability for a section containing unnamed_addr symbols is an optimization, and if it is not safe, it needs to be disabled until we can figure out a way to make it safe.

Sorry for the delay in updating this.

In D68101#1766359, @rjmccall wrote:

I can't speak for the pragma authors, but I strongly doubt the pragma is intended to force all affected globals to go into a single section unit, since the division into section units is purely an object-file representation issue.

The feature certainly could be implemented for COFF, so I think changing the name of the command line option is reasonable. The strings in the .deplibs sections map to libraries in an implementation defined manner, so I refer to them as "specifiers". In COFF dependent libraries are specified via "directives" in the object files. It might be worth naming the option something like: --dependent-lib-directives (as directives works for the entries in an ELF .deplibs sections equally as well as specifiers) ?

In D68101#1766151, @nickdesaulniers wrote:

In D68101#1765665, @rjmccall wrote:

Given all that, this patch seems far too aggressive. While mergeable sections can be useful for optimizing arbitrary code that might not use a section, they are also extremely useful for optimizing the sorts of global tables that programmers frequently use explicit sections for. It seems to me that the right fix is to find the place that ensures that we don't put mergeable and non-mergeable objects in the same section unit (or at least conservatively makes the section unit non-mergeable) and fix it to consider entry size as well. That should be straightforward unless that place doesn't exist, in which case we have very serious problems.

Disagree (but I spent all day thinking about this). Going back to https://bugs.llvm.org/show_bug.cgi?id=31828, I think I solved the problem there incorrectly; we should be not marking the explicit section merge-able.

In D68101#1689622, @SjoerdMeijer wrote:

Just to make sure I haven't missed anything, I would like to take this patch and run some numbers, for which I need a little bit of time.

Hi All, Apologies that I missed the request for input here.

In D68101#1689622, @SjoerdMeijer wrote:

Hi Ben, this is not really my area of expertise, but it all starts to make some sense to me. I was expecting this transformation to happen earlier, but this is where the magic happens, and this probably where it belongs.
Just to make sure I haven't missed anything, I would like to take this patch and run some numbers, for which I need a little bit of time. If in the mean time someone with some more experience in this area here has a look too, that would be great....

Hi Sjoerd,

In D68101#1684828, @jmolloy wrote:

This feels like it could cause a pretty serious regression. This essentially disables global merging with -fdata-sections, which I know at least one linker relies upon for code size.

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