Page MenuHomePhabricator

Harmonizing attribute GNU/C++ spellings
ClosedPublic

Authored by aaron.ballman on Nov 29 2017, 3:07 PM.

Details

Summary

Based on discussions at the WG21 meeting in Albuquerque and follow-up email discussions, I believe the correct approach to exposing attributes from Clang is to provide them with GNU-style attribute spellings as well as C++-style [[]] spellings in the clang vendor namespace, when appropriate. If the attribute was originally provided by a different vendor, and Clang intends to be compatible with those semantics, then the attribute should only be provided with the spellings supported by the original vendor. Otherwise, any attributes provided only by Clang should be exposed as both a GNU-style and C++-style attribute as appropriate for the semantics of that attribute.

To that end, this patch adds a C++ spelling in the clang vendor namespace to a number of attributes. Similarly, it adds a GNU spelling to the lto_visibility_public attribute (which was the only one with a C++ spelling but not a GNU spelling).

Finally, it leaves the following attributes with only a GNU spelling, based on the given rationale:

align_value -- originally provided by Intel with only a GNU spelling

constant, cudart_builtin, device, device_builtin_surface_type, device_builtin_texture_type, host, launch_bounds, shared, nv_weak -- attributes specified by CUDA with only a GNU spelling (several are also ignored attributes)

opencl_unroll_hint, intel_reqd_sub_group_size, nosvm, ext_vector_type, reqd_work_group_size, work_group_size_hint, vec_type_hint -- attributes specified by OpenCL with only a GNU spelling

kernel -- specified by RenderScript

carries_dependency -- supported as a standards-based attribute, shouldn't be in a vendor namespace

enable_if, diagnose_if, guarded_by, pt_guarded_by, acquired_after, acquired_before, assert_exclusive_lock, assert_shared_lock, exclusive_trylock_function, shared_trylock_function, lock_returned, locks_excluded -- not easily used with the C++ spelling because the attribute requires naming function parameters (these might be good candidates to explore changing into type attributes in the future).

bounded -- currently ignored

*Please double-check my understanding of these attribute spellings.* It is possible that I've misunderstood attributes as being specified by other sources when in fact they are Clang extensions, or that something is listed as a Clang extension but is actually a compatibility attribute. If my understanding is correct, I'll add the rationales to Attr.td as comments alongside the attribute spelling so that we don't lose that information.

Because this is a mechanical change that only introduces new spellings, there are no proposed test cases for the patch.

Diff Detail

Event Timeline

aaron.ballman created this revision.Nov 29 2017, 3:07 PM
rsmith added inline comments.Nov 29 2017, 6:24 PM
include/clang/Basic/Attr.td
596

Hmm, should the clang static analyzer reuse the clang:: namespace, or should it get its own?

643

Does the custom parsing for this work for the C++11 attribute syntax?

1212–1222

I *think* these are a Clang invention rather than part of the ARM NEON intrinsics specification, but perhaps you could ask someone from ARM to confirm that.

Added @dcoughlin for opinions about the static analyzer, added @sbaranga and @jmolloy for questions about NEON.

include/clang/Basic/Attr.td
596

Good question, I don't have strong opinions on the answer here, but perhaps @dcoughlin does?

If we want to use a separate namespace for the analyzer, would we want to use that same namespace for any clang-tidy specific attributes? Or should clang-tidy get its own namespace? (Do we ever plan to execute clang-tidy through the clang driver? That might change our answer.)

643

Nope; I'll do this one separately (and make sure no other attributes similarly use custom parsing).

1212–1222

@sbaranga or @jmolloy -- do you happen to know the answer to this, or know someone who does?

dcoughlin added inline comments.
include/clang/Basic/Attr.td
596

How would this look if we added a special namespace for the clang static analyzer? Would this lead to duplication (say, [[clang_analyzer::analyzer_noreturn]]) so that we keep the "analyzer_" prefix for attribute((analyzer_noreturn))? Or could we have the "analyzer_" prefix only for GNU-style attributes but not for C++ (for example, [[clang_analyzer::noreturn]])?

As for clang-tidy, I think it probably makes sense for it to have its own namespace, but we should ask @alexfh.

aaron.ballman added inline comments.Dec 1 2017, 9:03 AM
include/clang/Basic/Attr.td
596

How would this look if we added a special namespace for the clang static analyzer? Would this lead to duplication (say, [[clang_analyzer::analyzer_noreturn]]) so that we keep the "analyzer_" prefix for attribute((analyzer_noreturn))? Or could we have the "analyzer_" prefix only for GNU-style attributes but not for C++ (for example, [[clang_analyzer::noreturn]])?

We have the ability to do whatever we'd like there. Given that the semantics are so similar to [[noreturn]], I think it would be reasonable to use [[clang_analyzer::noreturn]] and __attribute__((analyzer_noreturn)) if that's the direction you think is best.

As for clang-tidy, I think it probably makes sense for it to have its own namespace, but we should ask @alexfh.

I'm less enthusiastic about giving clang-tidy a vendor namespace that's separate from the static analyzer, should the need arise. My biggest concern there is that I would *really* like to see clang-tidy be more tightly integrated with the clang driver (so users don't have to manually execute a secondary tool). If that were to happen, then the user experience would be that there are two vendor namespaces both related to analyzer attributes.

That said, I would also not be opposed to putting all of these attributes under the clang vendor namespace and not having a separate vendor for the analyzer or clang-tidy.

dcoughlin added inline comments.Dec 1 2017, 4:20 PM
include/clang/Basic/Attr.td
596

I would be find with keeping all of these things under the clang namespace, too.

That said, I do think there is some value in having a namespace for analyzer attributes separate from clang proper because the namespace would make it more clear that the attribute doesn't affect code generation.

aaron.ballman added inline comments.Dec 4 2017, 1:40 PM
include/clang/Basic/Attr.td
596

I've changed this one back to the GNU spelling to give us time to decide how we want to handle analyzer attributes.

I'm not certain "does not affect codegen" is the correct measure to use for this, however. We have other attributes that muddy the water, such as annotate, or the format specifier attributes -- these don't (really) impact codegen in any way, but do impact more than just the analyzer. Given the integration of the analyzer with Clang (and the somewhat fluid nature of what is responsible for issuing diagnostics), I think we should proceed with caution on the idea of an analyzer-specific namespace.

However, do you have a list of attributes you think might qualify as being analyzer-only? I can make sure we leave those spellings alone in this patch.

643

I've rolled back the following attributes with custom parsing to only use the GNU spelling for right now: availability, objc_bridge_related, argument_with_type_tag, pointer_with_type_tag, type_tag_for_datatype.

1212–1222

Pinging this comment.

sbaranga added inline comments.Dec 5 2017, 2:07 AM
include/clang/Basic/Attr.td
1212–1222

Yes, these should be internal to clang and used to implement the ACLE specification (which defines the NEON intrinsics). The ACLE doesn't define these.

Here is a link to the latest spec: https://developer.arm.com/products/software-development-tools/compilers/arm-compiler-5/docs/101028/latest/1-preface

aaron.ballman marked 4 inline comments as done.Dec 7 2017, 12:01 PM
aaron.ballman added inline comments.
include/clang/Basic/Attr.td
1212–1222

Perfect, thank you @sbaranga!

aaron.ballman marked an inline comment as done.

Updated based on review feedback. This patch leaves off attributes with custom parsing, as well as analyzer_noreturn, so that we can handle them as special cases.

rsmith accepted this revision.Dec 7 2017, 12:48 PM

LGTM.

I think the LateParsed attributes are not going to work properly (because they'd be written in places where the function parameters are not in scope), so I wonder if we should avoid allowing them with [[clang::]] notation for now so we can choose to make them function type attributes in the future, but I'm happy with you making that call.

This revision is now accepted and ready to land.Dec 7 2017, 12:48 PM
alexfh added inline comments.Dec 8 2017, 9:30 AM
include/clang/Basic/Attr.td
596

An argument against clang_tidy and clang_analyzer vendor namespaces is that the choice of where to implement a certain check would be connected to adding an attribute in one or both of these namespaces, which would complicate things a bit. In case both clang-tidy and static analyzer use the same argument, we'd need to have duplicate attributes. I definitely don't think we need three noreturn attributes, for example.

aaron.ballman added inline comments.Dec 8 2017, 12:33 PM
include/clang/Basic/Attr.td
596

Yeah, that's basically the concern I was getting at -- it really ties our hands on where the various checks live in a syntactic fashion, and that seems like it doesn't help our users any. They don't usually care whether something is a clang-tidy check vs analyzer vs compiler proper.

aaron.ballman closed this revision.Dec 14 2017, 2:18 PM

Committed (with additional comments on deviating attributes) in r320752.