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

[Erich Keane via libc-commits]

erichkeane 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.

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