1627007 - Allow using generalized [[foo]] attributes instead of or in addition to __attribute__((foo))

Status: NEW

(Developer Infrastructure :: Source Code Analysis, defect, P3)

Description

[Steve Fink [:sfink] [:s:]]

I ran into a case at https://searchfox.org/mozilla-central/rev/7fba7adfcd695343236de0c12e8d384c9b7cd237/dom/media/webaudio/AudioWorkletNode.cpp#816 where gcc and clang disagree with where the attribute should be placed. It looks like you ran into a similar pattern for a different reason in bug 1487622 (or at least, it's setting an attribute on a lambda).

The gcc-based hazard analysis wants to share this attribute with the clang plugin analyses, but in order to do so I need some kind of ugly workaround:

#if defined(__GNUC__) && ! defined(__clang__)
# define AND_MUTABLE(macro) mutable macro
#else
# define AND_MUTABLE(macro) macro mutable
#endif
       portId = std::move(processorPortId)]()
            AND_MUTABLE(MOZ_CAN_RUN_SCRIPT_BOUNDARY) {

It turns out that it's straightforward for me to switch from using __attribute__((annotate("moz_stuff"))) to [[moz::annotate("stuff")]]. My comment on the change that adds this to sixgill:

  // Also allow the syntax [[moz::annotate("...")]], which is (1) spec'ed as
  // being ignored if unrecognized, (2) more flexible in where it may be
  // placed, (3) defined by the c++ standard, and therefore (4) usable in the
  // same way across gcc and clang, and independent of preprocessor directives.

Would it be possible for the clang plugin to accept this syntax as well? It might even allow removing the #ifdefs in mfbt/Attributes.h.

Comment 1

[Firefox Bug Husbandry Bot]

Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is P3 (Backlog,) indicating it has been triaged, the bug's Severity is being updated to S3 (normal.)

Comment 2

[Kagami Rosylight [:saschanaz] (they/them) (OOO until Dec 11)]

(In reply to Steve Fink [:sfink] [:s:] from comment #0)

It turns out that it's straightforward for me to switch from using __attribute__((annotate("moz_stuff"))) to [[moz::annotate("stuff")]].

Is it? It seems there is no way to add custom attribute like moz::annotate and currently there's only clang-specific clang::annotate. (As it can be seen here: https://github.com/llvm/llvm-project/blob/bf8054644de952179ffc54f430ecf221ef260282/clang/test/Sema/annotate.c#L5)

Comment 3

[Steve Fink [:sfink] [:s:]]

(In reply to Kagami :saschanaz from comment #2)

(In reply to Steve Fink [:sfink] [:s:] from comment #0)

It turns out that it's straightforward for me to switch from using __attribute__((annotate("moz_stuff"))) to [[moz::annotate("stuff")]].

Is it? It seems there is no way to add custom attribute like moz::annotate and currently there's only clang-specific clang::annotate. (As it can be seen here: https://github.com/llvm/llvm-project/blob/bf8054644de952179ffc54f430ecf221ef260282/clang/test/Sema/annotate.c#L5)

I may be misunderstanding your comment, but I'm only saying that it's easy for me. Or rather, it's easy with the gcc plugin API. I don't know whether it's easy or hard for clang. But https://github.com/llvm/llvm-project/blob/main/clang/examples/Attribute/Attribute.cpp makes it look like you just need to add an alternate spelling. (See its example of ParsedAttr::AS_CXX11, "plugin::example").

It probably issues an error or warning if you're running without the plugin, but it looks like we already guard on MOZ_CLANG_PLUGIN for that.

Comment 4

[Steve Fink [:sfink] [:s:]]

Ah. I did not understand your comment 2 properly -- I did not realize that [[clang::annotate("foo")]] works already, and produces the same result as __attribute__((annotate("foo"))) modulo differences in placement.

As it turns out, switching to these c++11-style attributes isn't that straightforward, because the spec disallows them in at least one case (lambda functions) but both clang and gcc have (different) ways to use __attribute__ successfully there.

So this might need to wait for c++23, which will add the needed support.

Comment 5

[Kagami Rosylight [:saschanaz] (they/them) (OOO until Dec 11)]

Thanks for the update, adding see also for better visibility.