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?

In D41497#963176, @tejohnson wrote:

Thanks this looks much nicer. Question below about the handling of the Optional though.

As an aside, it would be great to have similar handling in parseCachePruningPolicy(), which is used by gold-plugin and lld, and which gives an error when -1 is passed as the interval. But that doesn't need to be in this patch, just noting it here.

Addressed review comments

I have reworked this now, see: https://reviews.llvm.org/D41497.

In D41231#958554, @labath wrote:

If Teresa is fine with this, then I don't want to hold this up. However, I would recommend looking at other ways to implement this behavior in the long term.

Explicitly cast system_clock::now type as well.

Thanks for the great review comments.

Updated patch to address review comments

In D41231#956807, @tejohnson wrote:

Add a test? You can use llvm-lto to trigger this legacy interface - although it looks like no support was added to llvm-lto to invoke this interface. It should be straightforward to add that (see where it calls setCacheDir).

In D41231#956664, @labath wrote:

In D41231#955453, @bd1976llvm wrote:

I like your proposal.

However, as the cache code is shared between multiple api's and only the c api has this disabling behaviour for negative values a fix here is more appropriate..

I don't quite agree with that conclusion -- the fact we have one upper layer that supports feature X means that the lower layer needs to support it as well (the other front-ends can just choose not to use it).

I removed the comment change.

I like your proposal.

Hi Ruiu,

Missed a review comment. Now addressed.

Addressed review comments.

updated with review comments.

Updated as per review comments.
Apologies that it took some time to update the patch.

Updated the diff to address review comments.

Having thought about this I still think that we want this patch.

In D38137#877803, @ruiu wrote:

First of all, could you read https://llvm.org/docs/CodingStandards.html and follow that style? One of the easiest way is to run clang-format-diff on your patch so that everything in your patch is formatted automatically.

Check out lld/trunk/test/ELF/common-gc3.s for a test case.

This case commonly arises where the section that is being patched is informational and the linker can't garbage collect it e.g .debug_info.

In D38136#877788, @ruiu wrote:

I'm not sure if this is an improvement. Essentially, we shouldn't call getSymVA on dead symbols (or, more strictly speaking, symbols pointing to dead sections), and I don't think we want to allow it.

I posted this to the mailing list (copy here):