[PATCH] D96914: [ELF] Add -z start-stop-gc to let __start_/__stop_ not retain C identifier name sections

Fri Nov 12 12:12:15 PST 2021

MaskRay added a comment.

In D96914#3128079 <https://reviews.llvm.org/D96914#3128079>, @jrtc27 wrote:

> In D96914#3128046 <https://reviews.llvm.org/D96914#3128046>, @MaskRay wrote:
>
>> In D96914#3127982 <https://reviews.llvm.org/D96914#3127982>, @tstellar wrote:
>>
>>> @MaskRay To be clear, I'm talking about reverting 6d2d3bd0a61f5fc7fd9f61f48bc30e9ca77cc619 <https://reviews.llvm.org/rG6d2d3bd0a61f5fc7fd9f61f48bc30e9ca77cc619> not this patch.  Again, I understand your motivations for keeping the default as-is, but we don't have consensus on this change and the policy for this project is to revert until a consensus can be reached.  Also, the fact the this patch was approved on the condition that the default would stay the same, but then the default was changed without discussion is a pretty strong reason to revert.  Can we please revert 6d2d3bd0a61f5fc7fd9f61f48bc30e9ca77cc619 <https://reviews.llvm.org/rG6d2d3bd0a61f5fc7fd9f61f48bc30e9ca77cc619> in trunk and continue the discussions in this thread?
>>>
>>> @jrtc27 @hvdijk  Do you have a suggested timeline for transitioning to the new default?
>>
>> @tstellar I know you are talking about 6d2d3bd0a61f5fc7fd9f61f48bc30e9ca77cc619 <https://reviews.llvm.org/rG6d2d3bd0a61f5fc7fd9f61f48bc30e9ca77cc619>, but I am afraid you may have missed some important discussions, e.g. https://bugs.llvm.org/show_bug.cgi?id=52384#c1
>> https://reviews.llvm.org/D96914#3115988 and https://reviews.llvm.org/D96914#3117455 .
>> I'd appreciate if others can read them first.
>> In particular, I have mentioned that this default switch been discussed with several groups, in the absence of jrtc27's agreement (who isn't a regular reviewer for lld/ELF code).
>
> I'm a regular contributor, pretty familiar with the code base, knowledgeable about ELF and reviewed this specific patch. There is no need to attempt to undermine me like that.
>
>> I don't think I necessarily agree with "revert until a consensus can be reached" if beyond reasonableness. The LLVM 17 release schedule proposed by @jrtc27 would just cause more trouble to FreeBSD if more newer software relies on the unfortunate GNU ld>=2015-10 behavior,
>
> So spit out a warning that the default is going to change if we detect --gc-sections + __start_/__stop_ symbols + no explicit -z (no)start-stop-gc? That's probably a good idea to do regardless.
>
>> even if it might make few LLVM IR code generation software (ldc which has a pending path out by its maintainer: https://github.com/ldc-developers/ldc/issues/3861) happy.
>
> Again, even if it newer versions of LDC get "fixed" (which they will anyway by virtue of using LLVM 13 for CodeGen), this does not "fix" older versions of LDC that exist in the wild.
>
>> I have repeatedly mentioned that delaying the switch will just cause more problems for distro like FreeBSD. I hope you can seek advice from @emaste and @dim for FreeBSD matter.
>> Let me emphasize again that distro-default `--gc-sections` isn't a common thing.
>
> We don't have a distro-default --gc-sections. The software affected is software that uses --gc-sections, or static libraries that are linked into binaries from a different source that use it.
>
>> On one hand, I am glad that LLD is so widely used; on the other hand, a decision may not be possible to make every group happy and we need to make right call.
>> Please don't overly emphasize a very rare thing just to make all other important use cases suffer.
>
> I would argue both are currently important, and that not breaking existing commonplace software is more important than optimising for a newer use case.

This isn't a new use case. GNU ld's `-z start-stop-gc` behavior (even if it did not have the option) was traditional and had been there for a very long time, until the 2015-10 commit moved it to `-z nostart-stop-gc`.

Upgrading compiler -> more aggressive on optimizations on exploiting UB -> software has to fix UB. This has been pretty common.
If you count, software needs to adapt after compiler upgrade is so common.
To prove that it's an important use case, please show more evidence that many software will break. (Also, since `--gc-sections` isn't distro default, we don't yet know how much doesn't even work with GNU ld's `--gc-sections`.)

(I shall note that declaring reserved identifiers `__start_` is UB in the first place. Well, the compiler has just always been just permissive.)

The traditional behavior also matches ld64's `section$start$xxx$yyy` behavior .

In D96914#3128133 <https://reviews.llvm.org/D96914#3128133>, @hvdijk wrote:

> In D96914#3128079 <https://reviews.llvm.org/D96914#3128079>, @jrtc27 wrote:
>
>> So spit out a warning that the default is going to change if we detect --gc-sections + __start_/__stop_ symbols + no explicit -z (no)start-stop-gc? That's probably a good idea to do regardless.
>
> That's a good idea when dealing with older compilers, but when up-to-date compilers are used, it would be nice if no warnings are emitted, as with up-to-date compilers in my opinion it's the code that should be changed as needed rather than the invocation. Is there a reliable way to tell that an object file was created using `SHF_GNU_RETAIN`-aware tools, regardless of whether the object file actually uses `SHF_GNU_RETAIN`?

In the FreeBSD llvm-project<13.0.0 library plus LLD 13.0.0 use case, unfortunately there is no portable and reliable way.

A warning would be incompatible with all existing and new instrumentations which do want GC.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96914