[clang] Add clang::unnamed_addr attribute that marks globals' address as not significant

[aeubanks]

Summary

This simply marks the global with the unnamed_addr IR attribute so it
be merged with other globals. This can break C++ pointer identity.

Add a warning -Wunnamed-addr that warns when putting the attribute on a
non-const or non-POD global since those can't be merged anyway.

Some users have said that they still want to take the address of many
auto-generated globals for table-driven parsing (many of which may be
equivalent), but do not care about address equality and
would like to save some binary size.

[aeubanks]

unsure if this should have an RFC or not

will add release notes if this is ok to go ahead with

[aeubanks]

and naming can be bikeshed. no_addrsig follows https://llvm.org/docs/Extensions.html#sht-llvm-addrsig-section-address-significance-table, unnamed_addr follows the IR attribute

[rnk]

So, I'm strongly in favor of the feature, but I want to get feedback on the name.

The major pro is that it maps directly to LLVM IR: the attribute does exactly what it says. The major con is that the name isn't really related to what the user wants to do, which is to merge duplicate constant data. This attribute is also not sufficient, since you also need -fdata-sections for ICF to fire on readonly data.

Some alternatives:

• no_addrsig
• enable_icf
• allow_icf
• allow_merging

If you force me to pick now, I would say let's go with [[clang:unnamed_addr]] because the search results for unnamed_addr already explain what it should do. Perhaps naming consistency is better than picking the most descriptive name.

[aeubanks]

merging doesn't have to be done at link time, the optimizer can also do it. so I'd rule out any names with icf since that's specifically the name of a linker feature

no_unique_address popped into my mind but that's already taken :). that's exactly what this is though. maybe no_unique_identity?

[aaron.ballman]

Hmmm, so this is effectively making a per-declaration version of -fmerge-all-constants? (How does this interact with the common attribute, if at all?)

clang/include/clang/Basic/AttrDocs.td

1416–1417

What happens for tentative definitions where the value isn't known? e.g.,

[[clang::unnamed_addr]] int i1, i2;

What happens if the types are similar but not the same?

[[clang::unnamed_addr]] signed int i1 = 32;
[[clang::unnamed_addr]] unsigned int i2 = 32;

Should we diagnose taking the address of such an attributed variable so users have some hope of spotting the non-conforming situations?

Does this attribute have impacts across translation unit boundaries (perhaps only when doing LTO) or only within a single TU?

What does this attribute do in C++ in the presence of constructors and destructors? e.g.,

struct S {
  S();
  ~S();
};

[[clang::unnamed_addr]] S s1, s2; // Are these merged and there's only one ctor/dtor call?

[aeubanks]

clang/include/clang/Basic/AttrDocs.td

1416–1417

globals are only mergeable if they're known to be constant and have the same value/size. this can be done at compile time only if the optimizer can see the constant values, or at link time

so nothing would happen in any of the cases you've given.

but yeah that does imply that we should warn when the attribute is used on non const, non-POD globals. I'll update this patch to do that

as mentioned in the description, we actually do want to take the address of these globals for table-driven parsing, but we don't care about identity equality

[aaron.ballman]

clang/include/clang/Basic/AttrDocs.td

1416–1417

globals are only mergeable if they're known to be constant and have the same value/size. this can be done at compile time only if the optimizer can see the constant values, or at link time

so nothing would happen in any of the cases you've given.

Ahhhh that's good to know. So I assume we *will* merge these?

struct S {
  int i, j;
  float f;
};

[[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
[[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
[[clang::unnamed_addr]] const S s3 = s2;

but yeah that does imply that we should warn when the attribute is used on non const, non-POD globals. I'll update this patch to do that

Thank you, I think that will be more user-friendly

as mentioned in the description, we actually do want to take the address of these globals for table-driven parsing, but we don't care about identity equality

Yeah, I still wonder if we want to diagnose just the same -- if the address is never taken, there's not really a way to notice the optimization, but if the address is taken, you basically get UB (and I think we should explicitly document it as such). Given how easy it is to accidentally take the address of something (like via a reference in C++), I think we should warn by default, but still have a warning group for folks who want to live life dangerously.

[aeubanks]

clang/include/clang/Basic/AttrDocs.td

1416–1417

globals are only mergeable if they're known to be constant and have the same value/size. this can be done at compile time only if the optimizer can see the constant values, or at link time

so nothing would happen in any of the cases you've given.

Ahhhh that's good to know. So I assume we *will* merge these?

struct S {
  int i, j;
  float f;
};

[[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
[[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
[[clang::unnamed_addr]] const S s3 = s2;

yeah those are merged even just by clang

struct S {
  int i, j;
  float f;
};

[[clang::unnamed_addr]] const S s1 = { 1, 2, 3.0f };
[[clang::unnamed_addr]] const S s2 = { 1, 2, 3.0f };
[[clang::unnamed_addr]] const S s3 = s2;

const void * f1() {
  return &s1;
}

const void * f2() {
  return &s2;
}

const void * f3() {
  return &s3;
}

$ ./build/rel/bin/clang++ -S -emit-llvm -o - -O2 /tmp/a.cc

but yeah that does imply that we should warn when the attribute is used on non const, non-POD globals. I'll update this patch to do that

Thank you, I think that will be more user-friendly

as mentioned in the description, we actually do want to take the address of these globals for table-driven parsing, but we don't care about identity equality

Yeah, I still wonder if we want to diagnose just the same -- if the address is never taken, there's not really a way to notice the optimization, but if the address is taken, you basically get UB (and I think we should explicitly document it as such). Given how easy it is to accidentally take the address of something (like via a reference in C++), I think we should warn by default, but still have a warning group for folks who want to live life dangerously.

I don't understand how there would be any UB. Especially if the user only dereferences them, and isn't comparing pointers, which is the requested use case.

[aaron.ballman]

clang/include/clang/Basic/AttrDocs.td

1416–1417

yeah those are merged even just by clang

Nice, thank you for the confirmation!

I don't understand how there would be any UB. Especially if the user only dereferences them, and isn't comparing pointers, which is the requested use case.

That's just it -- nothing prevents the user from taking the address and comparing the pointers, which is no longer defined behavior with this attribute. It would require a static analysis check to catch this problem unless the compiler statically warns on taking the address in the first place (IMO, we shouldn't assume users will use the attribute properly and thus need no help to do so). I was also thinking about things like accidental sharing across thread boundaries -- but perhaps that's fine because the data is constant. I was also thinking that this potentially breaks restrict semantics but on reflection... that seems almost intentional given the goal of the attribute. But things along these lines are what have me worried -- the language assumes unique locations for objects, so I expect there's going to be fallout when object locations are no longer unique. If we can remove sharp edges for users without compromising the utility of the attribute, I think that's beneficial. Or are you saying that warning like this would basically compromise the utility?

[aeubanks]

add warning

[aeubanks]

clang/include/clang/Basic/AttrDocs.td

1416–1417

I don't understand how there would be any UB. Especially if the user only dereferences them, and isn't comparing pointers, which is the requested use case.

That's just it -- nothing prevents the user from taking the address and comparing the pointers, which is no longer defined behavior with this attribute. It would require a static analysis check to catch this problem unless the compiler statically warns on taking the address in the first place (IMO, we shouldn't assume users will use the attribute properly and thus need no help to do so). I was also thinking about things like accidental sharing across thread boundaries -- but perhaps that's fine because the data is constant. I was also thinking that this potentially breaks restrict semantics but on reflection... that seems almost intentional given the goal of the attribute. But things along these lines are what have me worried -- the language assumes unique locations for objects, so I expect there's going to be fallout when object locations are no longer unique. If we can remove sharp edges for users without compromising the utility of the attribute, I think that's beneficial. Or are you saying that warning like this would basically compromise the utility?

when you say "undefined behavior" do you mean "it's unspecified what happens" or literally the C/C++ "undefined behavior" where the compiler can assume it doesn't happen?

I don't think there's any UB in the C/C++ "undefined behavior" sense, we're just dropping a C/C++ guarantee of unique pointer identity for certain globals.

Yes I believe the warning would compromise the utility since the underlying request behind this is a case where the user explicitly wants to take the address of these globals for table driven parsing but does not care about unique global identity. i.e. it's fine if we have duplicate addresses in the table as long as each entry points to the proper data.

[erichkeane]

clang/include/clang/Basic/AttrDocs.td

1416–1417

I think this IS causing undefined behavior, any program that assumes the addresses aren't the same (such as inserting addresses into a map, explicitly destructing/initializing/etc), or are comparing addresses are now exhibiting UB (in the purest of C++ senses). It isn't the 'taking the address' that is UB, it is comparing them, but unfortunately we don't have a good way to diagnose THAT. I believe what Aaron is getting at is that the taking of the addresses should be diagnosed, particularly if we end up taking said address less-obviously.

It DOES have to be a Static Analysis type diagnostic however, since I don't think it is accurate enough.

[aeubanks]

clang/include/clang/Basic/AttrDocs.td

1416–1417

perhaps I'm arguing too literally (or am just wrong), but even if the program behaves incorrectly due to comparisons of addresses now giving different results, that's not necessarily UB. even if a program were using pointers as a key into a map and two globals that previously were separate entries are now one, that's not necessarily UB, that's just a program behaving incorrectly.

unless there's a specific C/C++ rule that you have in mind that I'm missing?

I'm happy to add a warning that we can turn off internally if people agree that taking the address of an unnamed_addr global is dangerous in general, not sure if that should be default on or off

[erichkeane]

clang/include/clang/Basic/AttrDocs.td

1416–1417

The simplest one I can think of is something like a :

swap_containers(global1, global2); //swap that takes by ref, but doesn't check addresses

Warnings are obviously trivially disable-able, and I'd be fine with making it a really fine-grained warning group, but I think the dangers of this attribute are significant. MANY operations can have a precondition of "my parameters are different objects" that this can now cause you to trivially violate in a way that was never a problem before (since we give them different names!).

[rnk]

clang/include/clang/Basic/AttrDocs.td

1416–1417

I think, to Aaron's earlier question, warning on taking the address of such a global kind of defeats the point of the attribute. If you *don't* take the address of the constant global, LLVM will already mark it with unnamed_addr or local_unnamed_addr, and the attribute is usually unnecessary. You would only reach for this attribute if you are already taking the address of the global.

And, to try to elaborate more on the UB situation, I believe LLVM does perform optimizations like &gv1 == &gv2 -> false:
https://gcc.godbolt.org/z/3f5qd8vKr
This does present a soundness issue, but it's already a bit of an existing issue, since there are other ways to make globals alias, such as aliases.

[aeubanks]

clang/include/clang/Basic/AttrDocs.td

1416–1417

ah I misunderstood how ICF worked in regards to local_unnamed_addr. right now we will ICF globals that don't have their address taken across any translation unit.

$ cat /tmp/a.c
extern const int h;
extern const int g;
const int h = 42;
const int g = 42;

int f() {
  return h;
}

$ cat /tmp/b.c
extern const int g;

int f();

int main() {
  return g + f();
}

$ clang -o /tmp/a /tmp/a.c /tmp/b.c -O2 -fdata-sections -ffunction-sections -fuse-ld=lld -Wl,--icf=safe -Wl,--print-icf-sections -Wl,--verbose
...
ld.lld: ICF needed 2 iterations
selected section /usr/local/google/home/aeubanks/tmp/a-da430f.o:(.rodata.h)
  removing identical section /usr/local/google/home/aeubanks/tmp/a-da430f.o:(.rodata.g)
...

so yeah +1 to @rnk's comment. this change would only benefit globals that have their address taken.

[aaron.ballman]

clang/include/clang/Basic/AttrDocs.td

1416–1417

I think, to Aaron's earlier question, warning on taking the address of such a global kind of defeats the point of the attribute. If you *don't* take the address of the constant global, LLVM will already mark it with unnamed_addr or local_unnamed_addr, and the attribute is usually unnecessary. You would only reach for this attribute if you are already taking the address of the global.

Thanks for the information! That somehow makes me even *less* comfortable with the attribute because the only times you'd reach for it are precisely the times when it can bite you. Having some sort of static analysis check paired with it to help give it some guard rails for problematic uses would make me more comfortable. However, we certainly have other attributes that are "use at your own peril" without such guard rails, so I don't insist (but I still strongly encourage), but then I would insist on more comprehensive documentation calling out what misuse looks like and what potential problems come from misuse.

perhaps I'm arguing too literally (or am just wrong), but even if the program behaves incorrectly due to comparisons of addresses now giving different results, that's not necessarily UB. even if a program were using pointers as a key into a map and two globals that previously were separate entries are now one, that's not necessarily UB, that's just a program behaving incorrectly.

The reason why I believe it is UB is because everything else in the language model believes object addresses are distinct. So there's no way to determine what the impact is of two objects whose addresses which should be distinct but aren't, when that's observed by the program. It's undefined because we have no definition of what happens in this case. The program could be incorrect and just behave poorly, crash, cause data loss, etc. (As a concrete example, the restrict keyword effectively boils down to reasoning about whether two pointers overlap; if you pass the address of two distinct objects, they won't overlap but if they've been merged with this attribute, now they will overlap and the restrict requirements are broken.)

Regardless, I wasn't trying to get us hung up on UB as a standards term of art. I am worried about the user experience with the attribute. Optimization attributes that silently break program correctness do not provide a particularly good user experience and so if there's a practical way we can help users out without negatively impacting correct uses of the attribute, I think we should implement it. C and C++ already get a bad rap for safety and security because tooling is unhelpful, so I think we need to do better than we've done in the past in terms of diagnostic behavior.

[rnk]

clang/include/clang/Basic/AttrDocs.td

1416–1417

So, I think we disagree on the substance of the issue here, and perhaps we should try to get more opinions on Discourse. The feature request really is to have an optimization attribute to mark global constant data as mergeable by the linker, and for the user to promise that the address of the global is not significant (named).

clang/lib/Sema/SemaDecl.cpp

14524

I don't think isPODType really describes what we want. I think we want isConstantStorage which @dblaikie just added, which means essentially, can this global be placed in a readonly section.

14526

Should we ignore (drop) the attribute when we encounter this situation? Is it safe to mark mutable data unnamed_addr, or does it just do nothing?

[aeubanks]

clang/include/clang/Basic/AttrDocs.td

1416–1417

I'll post something to discourse

clang/lib/Sema/SemaDecl.cpp

14524

@dblaikie is this missing a check for a trivial constructor? using isConstantStorage the test is failing on const Foo by warning there when it shouldn't be

14526

it should be safe since merging mutable globals would be illegal even if the address is insignificant

[aaron.ballman]

clang/include/clang/Basic/AttrDocs.td

1416–1417

Sure, getting more feedback sounds reasonable to me. I should be clear -- I think I'm raising the bar on this attribute compared to previous optimization attributes, and that might be where some of this friction comes from. We have plenty of other optimization-related attributes that are "use at your own risk" without safety rails. However, I don't think that precedent is reasonable any longer in the face of growing resistance to using C and C++ for new projects specifically because tooling doesn't help catch safety and security related concerns, which are often the result of optimizer assumptions on incorrect code. So I'm asking for (what I think are) reasonable diagnostics to help people catch misuse, not trying to ask for heroics.

[aeubanks]

clang/include/clang/Basic/AttrDocs.td

1416–1417

I'm not seeing great binary size savings so I'm putting this off until somebody internally can give me a case where we can measure good binary size savings

[dblaikie]

clang/lib/Sema/SemaDecl.cpp

14524

(I realize this patch has been abandoned for now (probably worth actually marking it abandoned - I'm guessing folks are going to be wanting to look at outstanding phab reviews to try to get them cleaned out as we transition to github pull requests), just leaving some comments for the sake of it)

I think the isConstantStorage made some assumptions that the construction issues were already handled externally to the call... or you could pass in a bool to the second argument ExcludeCtor - but it's overly coarse/simplistic and I think just says "if you're not excluding the ctor, and the type is a CXXRecordDecl, then it's non-trivial" (so you'd pass in false for ExcludeCtor if you know the ctor you're using is non-trivial, otherwise pass in true). Because the function doesn't know which ctor you're using, so it's up to the caller to check. The dtor is checked, however (if ExcludeDtor is false).

clang/include/clang/Basic/Attr.td

 
 def NoUniqueAddress : InheritableAttr, TargetSpecificAttr<TargetItaniumCXXABI> {
   let Spellings = [CXX11<"", "no_unique_address", 201803>];
   let Subjects = SubjectList<[NonBitField], ErrorDiag>;
   let Documentation = [NoUniqueAddressDocs];
   let SimpleHandler = 1;
 }
 
+def UnnamedAddr : InheritableAttr {
+  let Spellings = [Clang<"unnamed_addr">];
+  let Subjects = SubjectList<[GlobalVar], ErrorDiag>;
+  let Documentation = [UnnamedAddrDocs];
+  let SimpleHandler = 1;
+}
+
 def ReturnsTwice : InheritableAttr {
   let Spellings = [GCC<"returns_twice">];
   let Subjects = SubjectList<[Function]>;
   let Documentation = [Undocumented];
   let SimpleHandler = 1;
 }
 
 def DisableTailCalls : InheritableAttr {

clang/include/clang/Basic/AttrDocs.td

.. code-block:: c++
   };
   static_assert(sizeof(my_vector<int, std::allocator<int>>) == sizeof(int*));
 
 ``[[no_unique_address]]`` is a standard C++20 attribute. Clang supports its use
 in C++11 onwards.
   }];
 }
 
+def UnnamedAddrDocs : Documentation {
+  let Category = DocCatField;
+  let Content = [{
+The ``clang::unnamed_addr`` attribute specifies that the address of a global is
+not significant. This allows global constants with the same contents to be
+merged. This can break global pointer identity, i.e. two different globals have
+the same address.
+
+Example usage:
+
+.. code-block:: c
+
+  // i and j may have the same address.
+  [[clang::unnamed_addr]] const int i = 42;
+  [[clang::unnamed_addr]] const int j = 42;
+  }];
+}
+
 def ObjCRequiresSuperDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 Some Objective-C classes allow a subclass to override a particular method in a
 parent class but expect that the overriding method also calls the overridden
 method in the parent class. For these cases, we provide an attribute to
 designate that a method requires a "call to ``super``" in the overriding
 method in the subclass.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_attribute_dll_ambiguous_default_ctor : Error<
   "'__declspec(dllexport)' cannot be applied to more than one default constructor in %0">;
 def err_attribute_weakref_not_static : Error<
   "weakref declaration must have internal linkage">;
 def err_attribute_weakref_not_global_context : Error<
   "weakref declaration of %0 must be in a global context">;
 def err_attribute_weakref_without_alias : Error<
   "weakref declaration of %0 must also have an alias attribute">;
+def warn_attribute_unnamed_addr : Warning<
+  "unnamed_addr should only be used on const POD (plain old data) globals">,
+  InGroup<DiagGroup<"unnamed-addr">>;
 def err_alias_not_supported_on_darwin : Error <
   "aliases are not supported on darwin">;
 def warn_attribute_wrong_decl_type_str : Warning<
   "%0%select{ attribute|}1 only applies to %2">, InGroup<IgnoredAttributes>;
 def err_attribute_wrong_decl_type_str : Error<
   warn_attribute_wrong_decl_type_str.Summary>;
 def warn_attribute_wrong_decl_type : Warning<
   "%0%select{ attribute|}1 only applies to %select{"

clang/lib/CodeGen/CodeGenModule.cpp

#endif
 
   // If it is in a read-only section, mark it 'constant'.
   if (const SectionAttr *SA = D->getAttr<SectionAttr>()) {
     const ASTContext::SectionInfo &SI = Context.SectionInfos[SA->getName()];
     if ((SI.SectionFlags & ASTContext::PSF_Write) == 0)
       GV->setConstant(true);
   }
 
+  if (D->hasAttr<UnnamedAddrAttr>())
+    GV->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
+
   CharUnits AlignVal = getContext().getDeclAlign(D);
   // Check for alignment specifed in an 'omp allocate' directive.
   if (std::optional<CharUnits> AlignValFromAllocate =
           getOMPAllocateAlignment(D))
     AlignVal = *AlignValFromAllocate;
   GV->setAlignment(AlignVal.getAsAlign());
 
   // On Darwin, unlike other Itanium C++ ABI platforms, the thread-wrapper

clang/lib/Sema/SemaDecl.cpp

if (PragmaClangRodataSection.Valid)
           Context, PragmaClangRodataSection.SectionName,
           PragmaClangRodataSection.PragmaLocation));
     if (PragmaClangRelroSection.Valid)
       VD->addAttr(PragmaClangRelroSectionAttr::CreateImplicit(
           Context, PragmaClangRelroSection.SectionName,
           PragmaClangRelroSection.PragmaLocation));
   }
 
+  if (VD->hasGlobalStorage() && VD->isThisDeclarationADefinition() &&
+      VD->hasAttr<UnnamedAddrAttr>() &&
+      (!VD->getType().isPODType(getASTContext()) ||
+       !VD->getType().isConstQualified())) {
+    Diag(VD->getLocation(), diag::warn_attribute_unnamed_addr);
+  }
+
   if (auto *DD = dyn_cast<DecompositionDecl>(ThisDecl)) {
     for (auto *BD : DD->bindings()) {
       FinalizeDeclaration(BD);
     }
   }
 
   checkAttributesAfterMerging(*this, *VD);
 

clang/test/CodeGen/unnamed-addr.c

+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -O1 -emit-llvm -o - -disable-llvm-passes %s | FileCheck %s
+
+// CHECK: @i = unnamed_addr constant i32 8
+
+[[clang::unnamed_addr]] const int i = 8;

clang/test/SemaCXX/unnamed-addr.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct Foo {
+  int a;
+};
+
+struct Bar {
+  int a;
+  Bar();
+};
+
+[[clang::unnamed_addr]] const int i = 8;
+
+[[clang::unnamed_addr]] int i2 = 8; // expected-warning{{unnamed_addr should only be used on const POD (plain old data) globals}}
+
+[[clang::unnamed_addr]] const Foo j = {2};
+
+[[clang::unnamed_addr]] Foo j2 = {2}; // expected-warning{{unnamed_addr should only be used on const POD (plain old data) globals}}
+
+[[clang::unnamed_addr]] const Bar k; // expected-warning{{unnamed_addr should only be used on const POD (plain old data) globals}}