aaron.ballman added a comment. In D126324#3537373 <https://reviews.llvm.org/D126324#3537373>, @wanders wrote:
> In D126324#3536785 <https://reviews.llvm.org/D126324#3536785>, @aaron.ballman > wrote: > >> The changes so far look sensible, but I think we should add some more tests >> for a few situations. 1) Using a const weak symbol as a valid initializer >> should be diagnosed (with a warning? with an error?) so users are alerted to >> the behavioral quirks. 2) Using a const weak symbol in a constant expression >> context should probably be an error, right? e.g., > > I do think we have already diagnose/fail on all the relevant cases here. And > have tests for them, like > > extern const int weak_int __attribute__((weak)); > const int weak_int = 42; > int weak_int_test = weak_int; // expected-error {{not a compile-time > constant}} > > in clang/test/Sema/const-eval.c > > But I'll have another look to make sure we have proper coverage. Thanks for double-checking it -- if we've got reasonable coverage, then that's great (nothing else needs to happen). >> Also, this definitely could use a release note so users know about the >> behavioral change. > > Happy to write one, but not sure what it should say. > The "only" change in behavior is that frontend no longer tells backend it is > allowed to optimize based on initializer value. Frontend behavior is intended > to be kept exactly the same (it already is restrictive enough). I thought this was more disruptive than it actually is, so I feel less strongly. But if you wanted to add one, I'd put it under the `Attribute Changes in Clang` heading and something along the lines of what you just wrote about the backend no longer being allowed to optimize in some circumstances. >> Do you have any ideas on how much code this change is expected to break? (Is >> it sufficient enough that we want to have an explicit deprecation period of >> the existing behavior before we switch to the new behavior?) > > I don't really know much code out there would be affected. As mentioned in > the discourse thread > https://discourse.llvm.org/t/weak-attribute-semantics-on-const-variables/62311/7 > I did some grepping in open source projects I could find. This was biased > towards looking at C code. > I could see two different uses of const+weak: > > 1. For defining a constant in a header file (which makes that symbol end up > in many object files, making it weak avoids multiple definition link errors) > 2. To define some default configuration/identificationstring which is > overridden by a strong symbol > > There might of course be other uses of weak that I'm not aware of or could > find. > > But these two cases I want to think are fine with the proposed change: > > For 1. we wouldn't alter behavior of code just speed due to optimization > being prevented. (I think these uses want to use "selectany" attribute > instead) > > For 2. this change should actually fix a bug where functions in the same > translation unit that provided the default was using the default rather than > overridden value. > > Here are some examples I found: > In u-boot they do this > https://source.denx.de/u-boot/u-boot/-/blob/master/common/hwconfig.c#L71 and > I think (havn't verified) clang/llvm would optimize the > `strlen(cpu_hwconfig)` on line 103 to `0`. So here I believe this code > currently has incorrect behavior when compiled with clang. > > Arduino for esp does this constant in header > https://github.com/esp8266/Arduino/blob/master/variants/generic/common.h#L79 > so that would cause an extra load of the value from memory for everyone > blinking the LED using the old deprecaded name for that pin. > > Here is a c++ example > https://github.com/ClickHouse/ClickHouse/blob/master/src/Common/Allocator.cpp#L11 > which I think it might not allow overriding the variable with a strong > definition as intended when compiling with clang. > > The case that motivated me to look into this is like: > > const char VERSION[] __attribute__ ((weak))= "devel"; > > int is_devel_version(void) { > return !strcmp (VERSION, "devel"); > } > > and when "VERSION" gets "weak_odr" linkage "is_devel_version" gets optimized > to "return 1". Even if VERSION gets replaced with a strong value later in > link phase. Excellent, thank you! I am comfortable moving forward with this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126324/new/ https://reviews.llvm.org/D126324 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
