[libc++] `no_unique_address` is reserved word in pre-C++20 dialects (non-conforming) #61196

JoeLoser · 2023-03-05T21:59:57Z

As we can see on this Godbolt link, even when compiling in earlier dialects than C++20 (such as C++17), we get a compiler error since no_unique_address is effectively a reserved word even in pre-C++20 dialects.

Some context: Clang supports [[no_unique_address] in all dialects for C++11 and later (I don't know why it doesn't support it in C++03 mode, even as __attribute__((no_unique_address))).

Failing to support a double-underscored no_unique_address means that no_unique_address encroaches into the user's namespace in C++11 through C++17 dialects which is not good.

One resolution would be if Clang could start supporting a __dundered form of no_unique_address that would be safely usable by the implementor, e.g. __attribute__((__no_unique_address__)).

The text was updated successfully, but these errors were encountered:

shafik · 2023-03-06T00:03:52Z

As per documentation: https://clang.llvm.org/docs/AttributeReference.html#no-unique-address this is supported in C++11 forward. I am looking at the PR: https://reviews.llvm.org/D63451 that implemented it and I don't see a rationale for supporting it as a extension in C++11.

CC @zygoloid who implemented and can perhaps talk to why it is supported pre C++20 and attributes code owner @erichkeane

zygoloid · 2023-03-06T01:44:57Z

I'm not sure I understand the question. It's clearly useful to support this in older versions, and it's conforming to do so. Indeed, the design of C++ attributes is intended precisely to permit compilers supporting attributes in older versions.

There's definitely a libc++ bug here -- it's not conforming for it to use __has_cpp_attribute(msvc::no_unique_address) like this because neither msvc nor no_unique_address are on the list of identifiers that a program shall not define before including a library header.

Now, Clang does support __no_unique_address__ as a synonym for no_unique_address, both in the attribute and in the __has_cpp_attribute check, but using that doesn't help, because this is a check for an MSVC feature, so what matters is what MSVC supports. And it looks like MSVC doesn't support a double-underscore version, so libc++ can't use that.

Presumably the proper fix for libc++ would be to guard this check with something like

#if !defined(msvc) && !defined(no_unique_address)

... but I suppose there's still the possibility that the user will #define no_unique_address or msvc between including the config header and including some other library header, so maybe even that doesn't work?

In any case, I don't see any Clang issue here. Retargeting as a libc++ bug.

shafik · 2023-03-06T04:46:26Z

@zygoloid maybe I am reading it wrong but it seems like macro.names would reserve the attribute-tokens and by supporting it as a extension we are encroaching on the user namespace early then folks would expect.

philnik777 · 2023-03-06T12:00:38Z

@zygoloid maybe I am reading it wrong but it seems like macro.names would reserve the attribute-tokens and by supporting it as a extension we are encroaching on the user namespace early then folks would expect.

The attribute-token s are the standard attributes, i.e. you can't #define no_unique_address in C++20-and-later, but you are allowed to do that in C++17. This essentially just means that the library is allowed to use the non-_Uglified versions of the attributes when they are in the standard. So you are free to do fun things like this, and as you see the compiler is happy to do that.

I think we should just remove the [[msvc::no_unique_address]] for now, since it's not supported by clang or GCC currently anyways.

shafik · 2023-03-06T19:41:11Z

I chatted w/ @zygoloid offline and we are saying the same thing, I was just missing some context. So I agree with @zygoloid original comment.

frederick-vs-ja · 2023-03-07T03:26:53Z

There's definitely a libc++ bug here -- it's not conforming for it to use __has_cpp_attribute(msvc::no_unique_address) like this because neither msvc nor no_unique_address are on the list of identifiers that a program shall not define before including a library header.

MSVC STL resolved this by preprocessor extensions (microsoft/STL#2649). Is the way suitable for libc++?

philnik777 · 2023-03-07T03:30:21Z

There's definitely a libc++ bug here -- it's not conforming for it to use __has_cpp_attribute(msvc::no_unique_address) like this because neither msvc nor no_unique_address are on the list of identifiers that a program shall not define before including a library header.

MSVC STL resolved this by preprocessor extensions (microsoft/STL#2649). Is the way suitable for libc++?

We don't support the Microsoft compiler anyways, so we can just ask the clang folks to add an __msvc__ spelling if [[msvc::no_unique_address]] ever gets implemented.

zygoloid · 2023-03-07T19:37:31Z

There's definitely a libc++ bug here -- it's not conforming for it to use __has_cpp_attribute(msvc::no_unique_address) like this because neither msvc nor no_unique_address are on the list of identifiers that a program shall not define before including a library header.

MSVC STL resolved this by preprocessor extensions (microsoft/STL#2649). Is the way suitable for libc++?

MSVC's approach is still broken: https://godbolt.org/z/cTYor9Ta6

It's also unsuitable for [[msvc::no_unique_address]] (unless the fallback path gets the same class layout a different way), because falling back on not using the attribute could result in an ABI difference between different TUs in the same program.

zygoloid · 2023-04-07T21:26:31Z

Reopening: the original issue reported here hasn't been fixed yet.

…lify-attributes This adds a list of attributes which can be pretty to be able to reject attributes which were introduced in a later C++ standard. Fixes llvm#61196 Reviewed By: Mordante, #libc Spies: mikhail.ramalho, jdoerfert, libcxx-commits Differential Revision: https://reviews.llvm.org/D145508

frederick-vs-ja · 2023-09-09T02:43:10Z

I think I've roughly completed MSVC's approach (microsoft/STL#3760).

If we apply that approach to libc++, there'll 35 files to be modified now (search result)...

philnik777 · 2023-09-09T02:47:54Z

I think I've roughly completed MSVC's approach (microsoft/STL#3760).

If we apply that approach to libc++, there'll 35 files to be modified now (search result)...

The attributes themselves are already _Ugly. The only thing that hasn't been fixed is when checking for attribute availability.

frederick-vs-ja · 2023-09-09T07:36:32Z

The attributes themselves are already _Ugly. The only thing that hasn't been fixed is when checking for attribute availability.

However, when the non-_Uglified attribute token msvc::no_unique_address is actually used (although currently it is never), #define msvc sth can still be problematic.

no_unique_address is perhaps OK itself since the attributes are unlikely to be used in pre-C++20 modes.

philnik777 · 2023-09-09T12:59:07Z

The attributes themselves are already _Ugly. The only thing that hasn't been fixed is when checking for attribute availability.

However, when the non-_Uglified attribute token msvc::no_unique_address is actually used (although currently it is never), #define msvc sth can still be problematic.

no_unique_address is perhaps OK itself since the attributes are unlikely to be used in pre-C++20 modes.

As I said before, IMO Clang should just accept some _Ugly version of the msvc attributes. That's how it's done for gnu and clang attributes, and makes this all trivial to fix.

cor3ntin · 2023-09-09T18:02:36Z

@philnik777 we do not support the attribute at all (yet), so it is difficile to preemptively do that. and actually, we can't do that for any attributes we don't support as it is an open set

philnik777 · 2023-09-09T18:38:59Z

@philnik777 we do not support the attribute at all (yet), so it is difficile to preemptively do that. and actually, we can't do that for any attributes we don't support as it is an open set

Yes, I'm aware of that. I just meant that for any attribute that clang accepts it should also accept an _Ugly alias, since that doesn't look like a ton of effort to me for the compiler but makes the life of standard library maintainers so much easier.

zygoloid · 2023-09-10T19:52:30Z

This is not a clang issue, the problem is in libc++. Clang changes won't help.

philnik777 · 2023-09-10T20:07:35Z

This is not a clang issue, the problem is in libc++. Clang changes won't help.

Yes, this is a libc++ issue. But I don't want to push and pop macros all over the place once clang supports [[msvc::no_unique_address]]. There should just be [[__msvc__::__no_unique_address__]] just like there are [[_Clang::__whatever__]] and [[__gnu__::__whatever__]] so libraries have it easier.

zygoloid · 2023-09-10T20:25:19Z

Clang already supports that for all its attributes. The issue is that MSVC does not, so if we want to support MSVC, then that doesn't help us. If we don't want to support MSVC then we should just remove all the msvc::no_unique_address code from libc++ to fix this bug. Then we can implement something clang-specific once #65675 is merged, but that's not part of this bug.

cor3ntin · 2023-09-11T08:03:14Z

Agreed. TBH, @frederick-vs-ja solution of using pragma push seems like the best solution here.

github-actions bot added the new issue label Mar 5, 2023

JoeLoser added the clang Clang issues not falling into any other category label Mar 5, 2023

zygoloid added libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. and removed clang Clang issues not falling into any other category labels Mar 6, 2023

EugeneZelenko removed the new issue label Mar 6, 2023

frederick-vs-ja mentioned this issue Mar 8, 2023

<yvals_core.h>: Conformance issue on [[msvc::known_semantics]] microsoft/STL#2645

Closed

philnik777 closed this as completed in c6afeda Apr 7, 2023

zygoloid reopened this Apr 7, 2023

cor3ntin mentioned this issue Sep 9, 2023

Implement [[msvc::no_unique_address]] #65675

Merged

philnik777 changed the title ~~[Clang] no_unique_address is reserved word in pre-C++20 dialects (non-conforming)~~ [libc++] no_unique_address is reserved word in pre-C++20 dialects (non-conforming) Sep 10, 2023

rnk mentioned this issue May 22, 2024

[libc++] Use [[no_unique_address]] in place of __compressed_pair to reduce compile time and debug info size #93069

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc++] `no_unique_address` is reserved word in pre-C++20 dialects (non-conforming) #61196

[libc++] `no_unique_address` is reserved word in pre-C++20 dialects (non-conforming) #61196

JoeLoser commented Mar 5, 2023

shafik commented Mar 6, 2023

zygoloid commented Mar 6, 2023 •

edited by VoltrexKeyva

Loading

shafik commented Mar 6, 2023 •

edited

Loading

philnik777 commented Mar 6, 2023

shafik commented Mar 6, 2023

frederick-vs-ja commented Mar 7, 2023 •

edited

Loading

philnik777 commented Mar 7, 2023

zygoloid commented Mar 7, 2023

zygoloid commented Apr 7, 2023

frederick-vs-ja commented Sep 9, 2023

philnik777 commented Sep 9, 2023

frederick-vs-ja commented Sep 9, 2023

philnik777 commented Sep 9, 2023

cor3ntin commented Sep 9, 2023

philnik777 commented Sep 9, 2023

zygoloid commented Sep 10, 2023

philnik777 commented Sep 10, 2023

zygoloid commented Sep 10, 2023

cor3ntin commented Sep 11, 2023

[libc++] no_unique_address is reserved word in pre-C++20 dialects (non-conforming) #61196

[libc++] no_unique_address is reserved word in pre-C++20 dialects (non-conforming) #61196

Comments

JoeLoser commented Mar 5, 2023

shafik commented Mar 6, 2023

zygoloid commented Mar 6, 2023 • edited by VoltrexKeyva Loading

shafik commented Mar 6, 2023 • edited Loading

philnik777 commented Mar 6, 2023

shafik commented Mar 6, 2023

frederick-vs-ja commented Mar 7, 2023 • edited Loading

philnik777 commented Mar 7, 2023

zygoloid commented Mar 7, 2023

zygoloid commented Apr 7, 2023

frederick-vs-ja commented Sep 9, 2023

philnik777 commented Sep 9, 2023

frederick-vs-ja commented Sep 9, 2023

philnik777 commented Sep 9, 2023

cor3ntin commented Sep 9, 2023

philnik777 commented Sep 9, 2023

zygoloid commented Sep 10, 2023

philnik777 commented Sep 10, 2023

zygoloid commented Sep 10, 2023

cor3ntin commented Sep 11, 2023

[libc++] `no_unique_address` is reserved word in pre-C++20 dialects (non-conforming) #61196

[libc++] `no_unique_address` is reserved word in pre-C++20 dialects (non-conforming) #61196

zygoloid commented Mar 6, 2023 •

edited by VoltrexKeyva

Loading

shafik commented Mar 6, 2023 •

edited

Loading

frederick-vs-ja commented Mar 7, 2023 •

edited

Loading