[PATCH] D126324: [clang] Allow const variables with weak attribute to be overridden

[Anders Waldenborg via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
wanders marked 2 inline comments as done.
Closed by commit rGdd2362a8bab3: [clang] Allow const variables with weak 
attribute to be overridden (authored by wanders).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126324/new/

https://reviews.llvm.org/D126324

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGen/global-init.c
  clang/test/CodeGen/weak_constant.c

Index: clang/test/CodeGen/weak_constant.c
===
--- clang/test/CodeGen/weak_constant.c
+++ clang/test/CodeGen/weak_constant.c
@@ -1,13 +1,31 @@
 // RUN: %clang_cc1 -w -emit-llvm %s -O1 -o - | FileCheck %s
-// Check for bug compatibility with gcc.
+// This used to "check for bug compatibility with gcc".
+// Now it checks that that the "weak" declaration makes the value
+// fully interposable whereas a "selectany" one is handled as constant
+// and propagated.
 
+// CHECK: @x = weak {{.*}}constant i32 123
 const int x __attribute((weak)) = 123;
 
+// CHECK: @y = weak_odr {{.*}}constant i32 234
+const int y __attribute((selectany)) = 234;
+
 int* f(void) {
   return &x;
 }
 
 int g(void) {
-  // CHECK: ret i32 123
+  // CHECK: load i32, ptr @x
+  // CHECK-NOT: ret i32 123
   return *f();
 }
+
+int *k(void) {
+  return &y;
+}
+
+int l(void) {
+  // CHECK-NOT: load i32, ptr @y
+  // CHECK: ret i32 234
+  return *k();
+}
Index: clang/test/CodeGen/global-init.c
===
--- clang/test/CodeGen/global-init.c
+++ clang/test/CodeGen/global-init.c
@@ -9,14 +9,20 @@
 
 // This should get normal weak linkage.
 int c __attribute__((weak))= 0;
-// CHECK: @c = weak{{.*}} global i32 0
+// CHECK: @c = weak global i32 0
 
-
-// Since this is marked const, it should get weak_odr linkage, since all
-// definitions have to be the same.
-// CHECK: @d = weak_odr constant i32 0
+// Even though is marked const, it should get still get "weak"
+// linkage, not "weak_odr" as the weak attribute makes it possible
+// that there is a strong definition that changes the value linktime,
+// so the value must not be considered constant.
+// CHECK: @d = weak constant i32 0
 const int d __attribute__((weak))= 0;
 
+// However, "selectany" is similar to "weak", but isn't interposable
+// by a strong definition, and should appear as weak_odr.
+// CHECK: @e = weak_odr constant i32 17
+const int e __attribute__((selectany)) = 17;
+
 // PR6168 "too many undefs"
 struct ManyFields {
   int a;
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -4899,12 +4899,8 @@
   if (Linkage == GVA_Internal)
 return llvm::Function::InternalLinkage;
 
-  if (D->hasAttr()) {
-if (IsConstantVariable)
-  return llvm::GlobalVariable::WeakODRLinkage;
-else
-  return llvm::GlobalVariable::WeakAnyLinkage;
-  }
+  if (D->hasAttr())
+return llvm::GlobalVariable::WeakAnyLinkage;
 
   if (const auto *FD = D->getAsFunction())
 if (FD->isMultiVersion() && Linkage == GVA_AvailableExternally)
Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -6477,3 +6477,69 @@
 The full documentation is available here: https://docs.microsoft.com/en-us/windows/win32/direct3dhlsl/sv-groupindex
   }];
 }
+
+def WeakDocs : Documentation {
+  let Category = DocCatDecl;
+  let Content = [{
+
+In supported output formats the ``weak`` attribute can be used to
+specify that a variable or function should be emitted as a symbol with
+``weak`` (if a definition) or ``extern_weak`` (if a declaration of an
+external symbol) `linkage
+`_.
+
+If there is a non-weak definition of the symbol the linker will select
+that over the weak. They must have same type and alignment (variables
+must also have the same size), but may have a different value.
+
+If there are multiple weak definitions of same symbol, but no non-weak
+definition, they should have same type, size, alignment and value, the
+linker will select one of them (see also selectany_ attribute).
+
+If the ``weak`` attribute is applied to a ``const`` qualified variable
+definition that variable is no longer consider a compiletime constant
+as its value can change during linking (or dynamic linking). This
+means that it can e.g no longer be part of an initializer expression.
+
+.. code-block:: c
+
+  const int ANSWER __attribute__ ((weak)) = 42;
+
+  /* This function may be replaced link-time */
+  __attribute__ ((weak)) void debug_log(const char *msg)
+  {
+  fprintf(stderr, "DE

[PATCH] D126324: [clang] Allow const variables with weak attribute to be overridden

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM! Please wait a day or two before landing in case @jyknight has additional 
comments, though. Thanks!


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

[PATCH] D126324: [clang] Allow const variables with weak attribute to be overridden

[Anders Waldenborg via Phabricator via cfe-commits]

wanders updated this revision to Diff 432669.
wanders added a comment.

Diff updated with release note + updated text in documentation as per 
jyknight's suggestion.

Tests for existing behavior added in separate patch: 
https://reviews.llvm.org/D126578


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126324/new/

https://reviews.llvm.org/D126324

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGen/global-init.c
  clang/test/CodeGen/weak_constant.c

Index: clang/test/CodeGen/weak_constant.c
===
--- clang/test/CodeGen/weak_constant.c
+++ clang/test/CodeGen/weak_constant.c
@@ -1,13 +1,31 @@
 // RUN: %clang_cc1 -w -emit-llvm %s -O1 -o - | FileCheck %s
-// Check for bug compatibility with gcc.
+// This used to "check for bug compatibility with gcc".
+// Now it checks that that the "weak" declaration makes the value
+// fully interposable whereas a "selectany" one is handled as constant
+// and propagated.
 
+// CHECK: @x = weak {{.*}}constant i32 123
 const int x __attribute((weak)) = 123;
 
+// CHECK: @y = weak_odr {{.*}}constant i32 234
+const int y __attribute((selectany)) = 234;
+
 int* f(void) {
   return &x;
 }
 
 int g(void) {
-  // CHECK: ret i32 123
+  // CHECK: load i32, ptr @x
+  // CHECK-NOT: ret i32 123
   return *f();
 }
+
+int *k(void) {
+  return &y;
+}
+
+int l(void) {
+  // CHECK-NOT: load i32, ptr @y
+  // CHECK: ret i32 234
+  return *k();
+}
Index: clang/test/CodeGen/global-init.c
===
--- clang/test/CodeGen/global-init.c
+++ clang/test/CodeGen/global-init.c
@@ -9,14 +9,20 @@
 
 // This should get normal weak linkage.
 int c __attribute__((weak))= 0;
-// CHECK: @c = weak{{.*}} global i32 0
+// CHECK: @c = weak global i32 0
 
-
-// Since this is marked const, it should get weak_odr linkage, since all
-// definitions have to be the same.
-// CHECK: @d = weak_odr constant i32 0
+// Even though is marked const, it should get still get "weak"
+// linkage, not "weak_odr" as the weak attribute makes it possible
+// that there is a strong definition that changes the value linktime,
+// so the value must not be considered constant.
+// CHECK: @d = weak constant i32 0
 const int d __attribute__((weak))= 0;
 
+// However, "selectany" is similar to "weak", but isn't interposable
+// by a strong definition, and should appear as weak_odr.
+// CHECK: @e = weak_odr constant i32 17
+const int e __attribute__((selectany)) = 17;
+
 // PR6168 "too many undefs"
 struct ManyFields {
   int a;
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -4899,12 +4899,8 @@
   if (Linkage == GVA_Internal)
 return llvm::Function::InternalLinkage;
 
-  if (D->hasAttr()) {
-if (IsConstantVariable)
-  return llvm::GlobalVariable::WeakODRLinkage;
-else
-  return llvm::GlobalVariable::WeakAnyLinkage;
-  }
+  if (D->hasAttr())
+return llvm::GlobalVariable::WeakAnyLinkage;
 
   if (const auto *FD = D->getAsFunction())
 if (FD->isMultiVersion() && Linkage == GVA_AvailableExternally)
Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -6477,3 +6477,69 @@
 The full documentation is available here: https://docs.microsoft.com/en-us/windows/win32/direct3dhlsl/sv-groupindex
   }];
 }
+
+def WeakDocs : Documentation {
+  let Category = DocCatDecl;
+  let Content = [{
+
+In supported output formats the ``weak`` attribute can be used to
+specify that a variable or function should be emitted as a symbol with
+``weak`` (if a definition) or ``extern_weak`` (if a declaration of an
+external symbol) `linkage
+`_.
+
+If there is a non-weak definition of the symbol the linker will select
+that over the weak. They must have same type and alignment (variables
+must also have the same size), but may have a different value.
+
+If there are multiple weak definitions of same symbol, but no non-weak
+definition, they should have same type, size, alignment and value, the
+linker will select one of them (see also selectany_ attribute).
+
+If the ``weak`` attribute is applied to a ``const`` qualified variable
+definition that variable is no longer consider a compiletime constant
+as its value can change during linking (or dynamic linking). This
+means that it can e.g no longer be part of an initializer expression.
+
+.. code-block:: c
+
+  const int ANSWER __attribute__ ((weak)) = 42;
+
+  /* This function may be replaced link-time */
+  __attribute__ ((weak)) void debug_log(const char *msg)
+  {
+  fpri

[PATCH] D126324: [clang] Allow const variables with weak attribute to be overridden

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D126324#3537373 , @wanders wrote:

> In 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

[PATCH] D126324: [clang] Allow const variables with weak attribute to be overridden

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D126324#3537282 , @jyknight wrote:

> In 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.
>
> I don't feel this is really necessary.

Why?

My thinking is: it's invalid to initialize a file scope variable in C from 
something other than a constant expression. These are not constant expressions. 
Why should the initialization work silently and maybe do something wrong?

>> 2. Using a const weak symbol in a constant expression context should 
>> probably be an error, right? e.g.,
>
> That's already the case (except, using a VLA is not an error in either 
> language mode). Clang previously _only_ treated weak symbols as 
> compile-time-known in the backend; the frontend has treated it as an 
> non-constant-evaluable variable for ages, and that behavior is unchanged by 
> this patch.

Ah, okay, then it sounds like we already have the behavior I was hoping for, 
great.

>> Also, this definitely could use a release note so users know about the 
>> behavioral change.
>>
>> 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?)
>
> Because it's only changing the optimizer behavior not the frontend constant 
> evaluator, I think it's pretty safe (which is also why I'm OK with it).

+1


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

[PATCH] D126324: [clang] Allow const variables with weak attribute to be overridden

[Anders Waldenborg via Phabricator via cfe-commits]

wanders added a comment.

In 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

[PATCH] D126324: [clang] Allow const variables with weak attribute to be overridden

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

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

I don't feel this is really necessary.

> 2. Using a const weak symbol in a constant expression context should probably 
> be an error, right? e.g.,

That's already the case (except, using a VLA is not an error in either language 
mode). Clang previously _only_ treated weak symbols as compile-time-known in 
the backend; the frontend has treated it as an non-constant-evaluable variable 
for ages, and that behavior is unchanged by this patch.

> Also, this definitely could use a release note so users know about the 
> behavioral change.
>
> 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?)

Because it's only changing the optimizer behavior not the frontend constant 
evaluator, I think it's pretty safe (which is also why I'm OK with it).




Comment at: clang/include/clang/Basic/AttrDocs.td:6527
+If an external declaration is marked weak and that symbol does not
+exist during linking (possibly dynamic) the linker will replace that
+with NULL.

Better said:
"""
If an external declaration is marked weak and that symbol does not
exist during linking (possibly dynamic) the address of the symbol
will evaluate to NULL.
"""

(function decls are just weird in that `&f` is treated as equivalent to `f`)


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

[PATCH] D126324: [clang] Allow const variables with weak attribute to be overridden

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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

  const int hmm __attribute__((weak)) = 12;
  int erp = hmm; // Error in C, dynamic init in C++?
  
  int main() {
int blerp = hmm; // OK in both languages
constexpr int derp = hmm; // Error in C++
int array[hmm]; // Error in both languages
  }

Also, this definitely could use a release note so users know about the 
behavioral change.

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?)




Comment at: clang/include/clang/Basic/Attr.td:2962
   let Subjects = SubjectList<[Var, Function, CXXRecord]>;
-  let Documentation = [Undocumented];
+  let Documentation = [WeakDocs];
   let SimpleHandler = 1;

THANK YOU!


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

[PATCH] D126324: [clang] Allow const variables with weak attribute to be overridden

[Anders Waldenborg via Phabricator via cfe-commits]

wanders updated this revision to Diff 431933.
wanders added a comment.

Diff updated to be git-clang-format clean and to (hopefully) accommodate for 
differences in test output on w64.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126324/new/

https://reviews.llvm.org/D126324

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGen/global-init.c
  clang/test/CodeGen/weak_constant.c

Index: clang/test/CodeGen/weak_constant.c
===
--- clang/test/CodeGen/weak_constant.c
+++ clang/test/CodeGen/weak_constant.c
@@ -1,13 +1,31 @@
 // RUN: %clang_cc1 -w -emit-llvm %s -O1 -o - | FileCheck %s
-// Check for bug compatibility with gcc.
+// This used to "check for bug compatibility with gcc".
+// Now it checks that that the "weak" declaration makes the value
+// fully interposable whereas a "selectany" one is handled as constant
+// and propagated.
 
+// CHECK: @x = weak {{.*}}constant i32 123
 const int x __attribute((weak)) = 123;
 
+// CHECK: @y = weak_odr {{.*}}constant i32 234
+const int y __attribute((selectany)) = 234;
+
 int* f(void) {
   return &x;
 }
 
 int g(void) {
-  // CHECK: ret i32 123
+  // CHECK: load i32, ptr @x
+  // CHECK-NOT: ret i32 123
   return *f();
 }
+
+int *k(void) {
+  return &y;
+}
+
+int l(void) {
+  // CHECK-NOT: load i32, ptr @y
+  // CHECK: ret i32 234
+  return *k();
+}
Index: clang/test/CodeGen/global-init.c
===
--- clang/test/CodeGen/global-init.c
+++ clang/test/CodeGen/global-init.c
@@ -9,14 +9,20 @@
 
 // This should get normal weak linkage.
 int c __attribute__((weak))= 0;
-// CHECK: @c = weak{{.*}} global i32 0
+// CHECK: @c = weak global i32 0
 
-
-// Since this is marked const, it should get weak_odr linkage, since all
-// definitions have to be the same.
-// CHECK: @d = weak_odr constant i32 0
+// Even though is marked const, it should get still get "weak"
+// linkage, not "weak_odr" as the weak attribute makes it possible
+// that there is a strong definition that changes the value linktime,
+// so the value must not be considered constant.
+// CHECK: @d = weak constant i32 0
 const int d __attribute__((weak))= 0;
 
+// However, "selectany" is similar to "weak", but isn't interposable
+// by a strong definition, and should appear as weak_odr.
+// CHECK: @e = weak_odr constant i32 17
+const int e __attribute__((selectany)) = 17;
+
 // PR6168 "too many undefs"
 struct ManyFields {
   int a;
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -4893,12 +4893,8 @@
   if (Linkage == GVA_Internal)
 return llvm::Function::InternalLinkage;
 
-  if (D->hasAttr()) {
-if (IsConstantVariable)
-  return llvm::GlobalVariable::WeakODRLinkage;
-else
-  return llvm::GlobalVariable::WeakAnyLinkage;
-  }
+  if (D->hasAttr())
+return llvm::GlobalVariable::WeakAnyLinkage;
 
   if (const auto *FD = D->getAsFunction())
 if (FD->isMultiVersion() && Linkage == GVA_AvailableExternally)
Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -6477,3 +6477,69 @@
 The full documentation is available here: https://docs.microsoft.com/en-us/windows/win32/direct3dhlsl/sv-groupindex
   }];
 }
+
+def WeakDocs : Documentation {
+  let Category = DocCatDecl;
+  let Content = [{
+
+In supported output formats the ``weak`` attribute can be used to
+specify that a variable or function should be emitted as a symbol with
+``weak`` (if a definition) or ``extern_weak`` (if a declaration of an
+external symbol) `linkage
+`_.
+
+If there is a non-weak definition of the symbol the linker will select
+that over the weak. They must have same type and alignment (variables
+must also have the same size), but may have a different value.
+
+If there are multiple weak definitions of same symbol, but no non-weak
+definition, they should have same type, size, alignment and value, the
+linker will select one of them (see also selectany_ attribute).
+
+If the ``weak`` attribute is applied to a ``const`` qualified variable
+definition that variable is no longer consider a compiletime constant
+as its value can change during linking (or dynamic linking). This
+means that it can e.g no longer be part of an initializer expression.
+
+.. code-block:: c
+
+  const int ANSWER __attribute__ ((weak)) = 42;
+
+  /* This function may be replaced link-time */
+  __attribute__ ((weak)) void debug_log(const char *msg)
+  {
+  fprintf(stderr, "DEBUG: %s\n", msg);
+  }
+
+  int main(int argc, const char **argv)
+  {
+  debug_

[PATCH] D126324: [clang] Allow const variables with weak attribute to be overridden

[Chris Lattner via Phabricator via cfe-commits]

lattner added a comment.

I'm not a competent reviewer for the patch, but +1 on aligning with GCC 
behavior here.  I don't recall the motivation for the original patch (apologies 
again for the terrible commit message).  This approach makes a lot of sense to 
me.


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

[PATCH] D126324: [clang] Allow const variables with weak attribute to be overridden

[Anders Waldenborg via Phabricator via cfe-commits]

wanders created this revision.
wanders added reviewers: lattner, jyknight, rsmith.
wanders added a project: clang.
Herald added a reviewer: aaron.ballman.
Herald added a project: All.
wanders requested review of this revision.
Herald added a subscriber: cfe-commits.

A variable with `weak` attribute signifies that it can be replaced with
a "strong" symbol link time. Therefore it must not emitted with
"weak_odr" linkage, as that allows the backend to use its value in
optimizations.

The frontend already considers weak const variables as
non-constant (note_constexpr_var_init_weak diagnostic) so this change
makes frontend and backend consistent.

This commit reverses the

  f49573d1 weak globals that are const should get weak_odr linkage.

commit from 2009-08-05 which introduced this behavior. Unfortunately
that commit doesn't provide any details on why the change was made.

This was discussed in
https://discourse.llvm.org/t/weak-attribute-semantics-on-const-variables/62311


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126324

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGen/global-init.c
  clang/test/CodeGen/weak_constant.c

Index: clang/test/CodeGen/weak_constant.c
===
--- clang/test/CodeGen/weak_constant.c
+++ clang/test/CodeGen/weak_constant.c
@@ -1,13 +1,31 @@
 // RUN: %clang_cc1 -w -emit-llvm %s -O1 -o - | FileCheck %s
-// Check for bug compatibility with gcc.
+// This used to "check for bug compatibility with gcc".
+// Now it checks that that the "weak" declaration makes the value
+// fully interposable whereas a "selectany" one is handled as constant
+// and propagated.
 
+// CHECK: @x = weak constant i32 123
 const int x __attribute((weak)) = 123;
 
+// CHECK: @y = weak_odr constant i32 234
+const int y __attribute((selectany)) = 234;
+
 int* f(void) {
   return &x;
 }
 
 int g(void) {
-  // CHECK: ret i32 123
+  // CHECK: load i32, ptr @x
+  // CHECK-NOT: ret i32 123
   return *f();
 }
+
+int* k(void) {
+  return &y;
+}
+
+int l(void) {
+  // CHECK-NOT: load i32, ptr @y
+  // CHECK: ret i32 234
+  return *k();
+}
Index: clang/test/CodeGen/global-init.c
===
--- clang/test/CodeGen/global-init.c
+++ clang/test/CodeGen/global-init.c
@@ -9,14 +9,22 @@
 
 // This should get normal weak linkage.
 int c __attribute__((weak))= 0;
-// CHECK: @c = weak{{.*}} global i32 0
+// CHECK: @c = weak global i32 0
 
 
-// Since this is marked const, it should get weak_odr linkage, since all
-// definitions have to be the same.
-// CHECK: @d = weak_odr constant i32 0
+// Even though is marked const, it should get still get "weak"
+// linkage, not "weak_odr" as the weak attribute makes it possible
+// that there is a strong definition that changes the value linktime,
+// so the value must not be considered constant.
+// CHECK: @d = weak constant i32 0
 const int d __attribute__((weak))= 0;
 
+// However, "selectany" is similar to "weak", but isn't interposable
+// by a strong definition, and should appear as weak_odr.
+// CHECK: @e = weak_odr constant i32 17
+const int e __attribute__((selectany)) = 17;
+
+
 // PR6168 "too many undefs"
 struct ManyFields {
   int a;
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -4893,12 +4893,8 @@
   if (Linkage == GVA_Internal)
 return llvm::Function::InternalLinkage;
 
-  if (D->hasAttr()) {
-if (IsConstantVariable)
-  return llvm::GlobalVariable::WeakODRLinkage;
-else
-  return llvm::GlobalVariable::WeakAnyLinkage;
-  }
+  if (D->hasAttr())
+return llvm::GlobalVariable::WeakAnyLinkage;
 
   if (const auto *FD = D->getAsFunction())
 if (FD->isMultiVersion() && Linkage == GVA_AvailableExternally)
Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -6477,3 +6477,69 @@
 The full documentation is available here: https://docs.microsoft.com/en-us/windows/win32/direct3dhlsl/sv-groupindex
   }];
 }
+
+def WeakDocs : Documentation {
+  let Category = DocCatDecl;
+  let Content = [{
+
+In supported output formats the ``weak`` attribute can be used to
+specify that a variable or function should be emitted as a symbol with
+``weak`` (if a definition) or ``extern_weak`` (if a declaration of an
+external symbol) `linkage
+`_.
+
+If there is a non-weak definition of the symbol the linker will select
+that over the weak. They must have same type and alignment (variables
+must also have the same size), but may have a different value.
+
+If there are multiple weak definitions of same symbol, but no n