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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:1416-1417
+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.
+
----------------
aeubanks wrote:
> aaron.ballman wrote:
> > 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?
> > ```
> 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
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158223