[libc-commits] [libc] Diagnose problematic uses of constructor/destructor attribute (PR #67673)

[Aaron Ballman via libc-commits]

AaronBallman wrote:

> > > (Was out of town...)
> > > As mentioned at [#67360 (comment)](https://github.com/llvm/llvm-project/pull/67360#issuecomment-1736187374) , I think a default error is probably not appropriate. A priority value <= 100 may compete with a runtime `__attribute__((constructor(X)))` but more often it will work. A warning is fine. GCC defaults to a warning and the attribute form with a priority is an obscure feature anyway, therefore it doesn't buy us much to make it an error. You can make `> 65535` an error, though.
> > 
> > 
> > I disagree -- we decided ages ago with init_priority that using these reserved priorities should be an error that can be ignored for the few cases where the reservation does not apply. We should be designing the Ux for these features around the majority of users, not the majority of people who will be expert user of the feature. Otherwise, we should not pretend these are reserved values -- it's far too easy to ignore a warning IMO.
> > That said, I'm abandoning this work regardless. It was unplanned work I took on only because I noticed how deficient the implementation was regarding diagnostics when I went to add documentation for the attribute in the first place. A lot of our attributes from 2000-2012 or so are similarly problematic, unfortunately. So I wouldn't be surprised to see this come up again in the future.
> 
> First, it would be a shame if you abandon this over this bit, there is plenty of value to this patch, even as far as the refactoring (which makes some other folks' patches easier to review!). While I agree with you that the error is appropriate, it doesn't seem practical at the moment thanks to GCC's implementation. Abandoning this patch means that those will NEVER be an error, so at least having this patch in gives us an opportunity to make this an error 'someday'.
> 
> IMO, a warning/Warning-as-default-error are both appropriate (with preference towards the latter) 'for now', and we should fight the battle of making this a hard error another time.

That doesn't solve the issue though -- we have `-Werror` bots for compiler-rt, so someone still needs to figure out how to beat CMake into shape even if we introduce it as a warning. I took several runs at the build system and was unsuccessful -- I can't repro locally on Windows and our precommit CI doesn't demonstrate the issue, so this requires making a number of speculative commits + watch the bots for fallout and that's where I ran out of time for the unexpected effort. I'm totally happy if someone wants to pick this up and run it across the finish line, warning-as-error or warning form. I personally think warning-as-error form is the correct approach though; "GCC doesn't err" isn't really compelling logic to me because GCC attributes (especially the older ones) have traditionally had underwhelming diagnostic behavior -- we should strive to do better when possible.

https://github.com/llvm/llvm-project/pull/67673