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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
