wanders added a comment.
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.
> 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).
> 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.
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
