Proposal: allow [[clang::attributes]] in Chromium without macros

[danakj]

Hello,

We currently use macros in absl (third_party/angle/third_party/abseil-cpp/absl/base/attributes.h) and in base (base/compiler_specific.h) to wrap attributes that may or may not exist for different compilers.

Chromium supports compilation with exactly two compilers: Clang and GCC. For attributes that GCC does not know, a Wattributes warning is emitted, and since we compile with Werror, it will fail to build. Thus, we use macros to conditionally define [[clang::attributes]] away in GCC.

Instead of requiring (long) include paths and (long and shouty) macros, I propose that we allow [[clang::attributes]] directly in Chromium code, except in places where additional compiler support is desired (specifically, things like third_party/dawn, and partition_alloc). Then, that we add -Wno-attributes to the cflags set when building with GCC.

This will allow for much more clear code (no indirection through macros), and much simpler reading and writing (no looking for headers, no looking through macros to see what they resolve to).

Are there other concerns that -Wno-attributes won't resolve? Thoughts?

Thanks,

danakj

[Jeremy Roman]

Most of the macros have a fairly clear 1:1 correspondence with clang attributes, like ABSL_ATTRIBUTE_LIFETIME_BOUND is [[clang::lifetimebound]]. Since the correspondence is relatively obvious (and if it's not, we could rename the macro to make it so), is this a large benefit?

It also deals with the case of clang version skew, though we're usually not so aggressive about adopting attributes that that is an issue.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaQr7h_p%3DhLTiebCQC0CADwF4YzQAEyafXB2JW6KNk6qng%40mail.gmail.com.

[danakj]

On Thu, Feb 15, 2024 at 4:31 PM Jeremy Roman <jbr...@chromium.org> wrote:

Most of the macros have a fairly clear 1:1 correspondence with clang attributes, like ABSL_ATTRIBUTE_LIFETIME_BOUND is [[clang::lifetimebound]]. Since the correspondence is relatively obvious (and if it's not, we could rename the macro to make it so), is this a large benefit?

Well, I think it's a small benefit that repeats a lot. Does that add up to a lot? These macros are a different dialect and I am testing if we really need that forked dialect anymore. I don't think that we do.

It also deals with the case of clang version skew, though we're usually not so aggressive about adopting attributes that that is an issue.

Yeah, for other toolchains that run an older version of clang, they can also use -Wno-attributes (or -Wno-error=attributes) which will generate the same code as they would otherwise, since when the attribute is not there, the macros remove it.

So this seems to be like a strictly more readable codebase.

I have only suggested this for [[clang::attributes]] not for other __attribute__((gnu attributes)) at this time, to limit the scope.

[Roland McGrath]

On Thu, Feb 15, 2024 at 1:36 PM danakj <dan...@chromium.org> wrote:

I have only suggested this for [[clang::attributes]] not for other __attribute__((gnu attributes)) at this time, to limit the scope.

I like the readability goal. I'd posit that the more limited-scope and easier first incremental step is to replace `MACRO_FOR_FOO` and `__attribute__((foo))` with `[[gnu::foo]]` since that syntax is accepted by all the compilers and (for the most part) Clang supports all the `gnu::` attributes that are much used.  ISTM that could most likely be done for all the GNU attributes without any cases arising where `-Wno-attribute` was needed.  Once the readability improvement from that is observed, it strengthens the case for following through for the Clang-specific attributes and making sure that `-Wno-attributes` plumbing is accessible to those who need it. 

[Peter Boström]

I'd be concerned that we're effectively adding another way of spelling out the same thing. If people are not used to working with clang attributes directly (i.e. used to working with absl/ or any other project that wrap these in macros) then spelling them out directly may even look weird. In this example I'd imagine that ABSL_ATTRIBUTE_LIFETIME_BOUND may read more familiar with [[clang:lifetimebound]] and I would assume that more people are used to working with the former. No hard take either way.

On Thu, Feb 15, 2024 at 1:36 PM danakj <dan...@chromium.org> wrote:

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaRumSSrodRonfAL%2Bco2RM772XpQ2hatiqw2eW36p4-aeQ%40mail.gmail.com.

[Peter Boström]

Actually ABSL_ATTRIBUTES_ is a mouthful. I would probably be happy to learn another way to spell it.

[Peter Kasting]

I'm not a big fan of this proposal.

For some attributes, we discover later that there's a reason to use a different spelling. [[no_unique_address]] was a case of this. I realize that is not a clang:: attribute (though there was discussion of what a clang:: version might do in clang-cl!), but I don't think that subtle distinction moots the underlying point I'm getting at, which is that having an abstraction over the spelling is often useful.

I also think we should _require_ clang only when its reasonably difficult to do otherwise. I have within the past year attempted to locally build with msvc, and will likely do so again. I'm not asking us to bend over backwards to support every compiler, but I think we should avoid gratuitous dependencies when doing so is reasonably trivial.

Using standard attributes like [[nodiscard]] is imo a nice win over NODISCARD. Using [[clang::foobar]] over FOOBAR? Not a win, imo.

PK

--

[David Benjamin]

No opinions either way on [[clang::lifetimebound]], but perhaps we could ask Abseil to consider plain ABSL_LIFETIME_BOUND? Skimming third_party/abseil-cpp/absl/base/attributes.h, I don't think the ATTRIBUTE parts of those names are pulling their weight.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAGFX3sGdUnRKQkQKgdSuMazScn-OmqkcAi1bz9jo3RfDOT0DAg%40mail.gmail.com.

[Peter Kasting]

+1

dmauro@ is a good internal person to run this by.

PK

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF8qwaBaTsbtx6nho-PwxLV_k9sy%2BDasVHfpuscqrJ_FOFOZQw%40mail.gmail.com.

[danakj]

On Thu, Feb 15, 2024 at 6:50 PM Peter Kasting <pkas...@google.com> wrote:

I'm not a big fan of this proposal.

For some attributes, we discover later that there's a reason to use a different spelling. [[no_unique_address]] was a case of this. I realize that is not a clang:: attribute (though there was discussion of what a clang:: version might do in clang-cl!), but I don't think that subtle distinction moots the underlying point I'm getting at, which is that having an abstraction over the spelling is often useful.

[[no_unique_address]] vs [[msvc::no_unique_address]] is an exception here that requires a macro because there is more than one way to spell it, and they mean different things. This is not representative of attributes in general, nor of [[clang::attributes]] at all. That one attribute (which is not part of this proposal) requires a macro does not imply we need to use macros for every attribute.

I also think we should _require_ clang only when its reasonably difficult to do otherwise. I have within the past year attempted to locally build with msvc, and will likely do so again. I'm not asking us to bend over backwards to support every compiler, but I think we should avoid gratuitous dependencies when doing so is reasonably trivial.

If we want to support MSVC that should be a separate discussion. Current policy is that we don't. The proposal here doesn't make things worse anyway, [[clang::attributes]] would disappear on MSVC just as on GCC.

[Adam Rice]

I would prefer to keep the macros. Some of those attributes are load-bearing and shouldn't just silently vanish. Also, in most cases use should be rare and the SHOUTINESS helps draw attention to the reader that something weird is going on.

Also, while our limited compiler support is unavoidable given the size of the project, I don't think it's something we should be proud of. It's still best practice to write portable code and confine and encapsulate unportable parts, and the attribute macro headers support that.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaTruV_LWR9p7E--jw1stBtGhMQi2_2jugdV_qu%3D_Wrpxw%40mail.gmail.com.

[Peter Kasting]

On Friday, February 16, 2024 at 8:02:47 AM UTC-8 danakj wrote:

On Thu, Feb 15, 2024 at 6:50 PM Peter Kasting <pkas...@google.com> wrote:

For some attributes, we discover later that there's a reason to use a different spelling. [[no_unique_address]] was a case of this. I realize that is not a clang:: attribute (though there was discussion of what a clang:: version might do in clang-cl!), but I don't think that subtle distinction moots the underlying point I'm getting at, which is that having an abstraction over the spelling is often useful.

[[no_unique_address]] vs [[msvc::no_unique_address]] is an exception here that requires a macro because there is more than one way to spell it, and they mean different things. This is not representative of attributes in general, nor of [[clang::attributes]] at all. That one attribute (which is not part of this proposal) requires a macro does not imply we need to use macros for every attribute.

Many clang attributes are similar to gcc attributes but behave in distinct ways. The format attributes (for printf safing) are examples of this. It's not clear to me that gcc would always want the [[gcc:xxx]] version of [[clang:xxx]] exactly, or that it would want nothing. My point was that when different compilers do different things, abstractions are useful to accommodate them.

I also think we should _require_ clang only when its reasonably difficult to do otherwise. I have within the past year attempted to locally build with msvc, and will likely do so again. I'm not asking us to bend over backwards to support every compiler, but I think we should avoid gratuitous dependencies when doing so is reasonably trivial.

If we want to support MSVC that should be a separate discussion. Current policy is that we don't. The proposal here doesn't make things worse anyway, [[clang::attributes]] would disappear on MSVC just as on GCC.

I'm not asking for MSVC support; I'm saying that removing this abstraction mechanism makes locally testing such things vastly harder, because now you need to touch every usage site to add a load-bearing use of an attribute, instead of adding a definition in a single central place.

I wonder if we are debating a solution without defining the problem well. My suspicion is that you were frustrated by ABSL_ATTRIBUTE_LIFETIME_BOUND having a long name and a long #include path. Those are reasonable frustrations. But this is not the only possible solution. Imagine a world where you could do:

#include "absl/attributes.h"

Foo(const T& t ABSL_LIFETIME_BOUND);

Is that world sufficiently good for you? If so, both changes needed to get there seem to me like they'd also benefit a lot of use cases, so I'm happy to join in pushing for them.

PK

[Daniel Cheng]

I think it'd be fine to have a shorter way to refer to these attributes (whether that's BASE_LIFETIME_BOUND or ABSL_LIFETIME_BOUND, or something else altogether).

I do think we should avoid writing these out literally: in spirit, I think it violates the "Nonstandard extensions to C++ may not be used unless otherwise specified." part of the C++ style guide.

Previous internal style discussion: http://shortn/_pPRrLvIBLY

Daniel

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/bd2f42c0-2fc7-4bc8-8c9a-ce47e42bc5adn%40chromium.org.

[Peter Kasting]

On Tue, Feb 20, 2024 at 12:24 PM Daniel Cheng <dch...@chromium.org> wrote:

I think it'd be fine to have a shorter way to refer to these attributes (whether that's BASE_LIFETIME_BOUND or ABSL_LIFETIME_BOUND, or something else altogether).

I do think we should avoid writing these out literally: in spirit, I think it violates the "Nonstandard extensions to C++ may not be used unless otherwise specified." part of the C++ style guide.

Previous internal style discussion: http://shortn/_pPRrLvIBLY

Ah, that's an interesting discussion.

Based on this thread, my proposal for attributes is that anything we care about should have a non-prefixed version (e.g. LIFETIME_BOUND) in base/compiler_specific.h, and we ban the Abseil versions where they overlap. I don't care whether our implementation of that version delegates to Abseil or is bespoke.

Dana, would that be sufficient for you? Are there other attributes you're interested in beyond [[clang::lifetimebound]]?

I will leave the proposal about having a top-level absl/ directory with forwarding headers hanging. I raised it in passing in the thread about WrapUnique and Marijn pointed at a past thread with some concerns, so it's not an obvious slam dunk. I still think it'd be a win on-net, but I have enough other irons in the fire that I'm not going to try to drive it. If someone else wants to formally propose, go for it (but use a separate top-level thread).

PK

[danakj]

On Wed, Feb 21, 2024 at 2:12 PM Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Feb 20, 2024 at 12:24 PM Daniel Cheng <dch...@chromium.org> wrote:

I think it'd be fine to have a shorter way to refer to these attributes (whether that's BASE_LIFETIME_BOUND or ABSL_LIFETIME_BOUND, or something else altogether).

I do think we should avoid writing these out literally: in spirit, I think it violates the "Nonstandard extensions to C++ may not be used unless otherwise specified." part of the C++ style guide.

Previous internal style discussion: http://shortn/_pPRrLvIBLY

Ah, that's an interesting discussion.

Based on this thread, my proposal for attributes is that anything we care about should have a non-prefixed version (e.g. LIFETIME_BOUND) in base/compiler_specific.h, and we ban the Abseil versions where they overlap. I don't care whether our implementation of that version delegates to Abseil or is bespoke.

Dana, would that be sufficient for you? Are there other attributes you're interested in beyond [[clang::lifetimebound]]?

This would be a huge improvement, yes. Lifetimebound is what is top of mind, along with ABSL_ATTRIBUTE_PURE_FUNCTION and ABSL_ATTRIBUTE_CONST_FUNCTION.

[Peter Kasting]

On Tue, Mar 5, 2024 at 11:47 AM danakj <dan...@chromium.org> wrote:

On Wed, Feb 21, 2024 at 2:12 PM Peter Kasting <pkas...@chromium.org> wrote:

Based on this thread, my proposal for attributes is that anything we care about should have a non-prefixed version (e.g. LIFETIME_BOUND) in base/compiler_specific.h, and we ban the Abseil versions where they overlap. I don't care whether our implementation of that version delegates to Abseil or is bespoke.

This would be a huge improvement, yes. Lifetimebound is what is top of mind, along with ABSL_ATTRIBUTE_PURE_FUNCTION and ABSL_ATTRIBUTE_CONST_FUNCTION.

Raised as a new thread.

PK 

[Peter Kasting]

Given that the proposal on the new thread was approved, I consider this one mooted.

PK