Page MenuHomePhabricator

Add SEMA checking to attribute 'target'
AbandonedPublic

Authored by erichkeane on Jul 17 2017, 4:53 PM.

Details

Summary

Attribute 'target' can take either features or architectures. This patch accomplishes this in a couple of ways:

1- It adds support to Targets.cpp for "isValidCPUName" and "isValidFeatureName". HOWEVER, the latter is only implemented for X86. I'm hoping to delay implementation of those until later (or get help on that).
2- Cleans up the 'target' entry in Attr.td to use a struct instead of a 'Pair', so that more data can be included.
3- Validates the attribute sufficiently.

Note that we currently do not have multi-versioning support (that is to come in a future patch) and SemaDeclAttr.cpp lacks knowledge of whether this is a multi-version, so this validation doesn't cover the multi-versioning case.

Diff Detail

Event Timeline

erichkeane created this revision.Jul 17 2017, 4:53 PM
aaron.ballman added inline comments.Jul 18 2017, 5:14 AM
include/clang/Basic/DiagnosticSemaKinds.td
2389–2393

All of these (including the one you didn't have to touch) should be using "ignoring" instead of "Ignoring".

2394

This name should have something to do with attributes. Might make sense to combine with warn_unsupported_target_attribute using a %select given the similarity in use case.

lib/Basic/Targets.cpp
1019–1022

I think this comment should be hoisted up to isValidCPUName().

6416

It's rather strange that this doesn't actually set the CPU, but I can't see that the original code did either, so maybe it's fine?

9617

You can drop the this, and I would put the CPU = Name onto its own line (not certain if that's a personal preference or a general community style).

lib/Sema/SemaDeclAttr.cpp
3008

Missing a space after the if.

3009

Should this be "arch=" to be consistent with the previous diagnostic on line 3001?

3011

Is the attribute valid when the architecture is empty?

3016

Can this be made const auto &?

3017

Insert a space before the comment.

test/Sema/attr-target.c
8

Please add a test case where there's a duplicate arch and an empty arch.

erichkeane marked 9 inline comments as done.

Thanks for the feedback Aaron. This should fix everything you had an issue with.

-Erich

aaron.ballman accepted this revision.Jul 18 2017, 8:28 AM

A few small nits, but otherwise LGTM

lib/Sema/SemaDeclAttr.cpp
2999–3000

I don't think this comment is needed (the code is pretty obvious), so I'd just drop it entirely.

3001–3002

enumerator casing should be Unsupported, Duplicate, Architecture.

This revision is now accepted and ready to land.Jul 18 2017, 8:28 AM
erichkeane marked 2 inline comments as done.Jul 18 2017, 8:34 AM

Thanks Aaron! Fixed those things and will be submitting soon. Also ran format on the function in SemaDeclAttr. For some reason it didn't right the first time.

echristo edited edge metadata.Jul 18 2017, 8:46 AM

If you wouldn't mind I'd like to see this separated out a bit -
a) I think we can make the attribute info a struct rather than a pair as a simple change first
b) setCPU split
c) let's see where we are after that?

Also the description makes it sound like you're adding support for the feature rather than fixing the terrible error diagnostics - or at least to my early morning brain. :)

Also, one inline comment so far.

Thanks!

-eric

test/Sema/attr-target.c
10

Precedence issue maybe, but I'd have expected this to error on the architecture name rather than the duplicate?

If you wouldn't mind I'd like to see this separated out a bit -
a) I think we can make the attribute info a struct rather than a pair as a simple change first
b) setCPU split
c) let's see where we are after that?

Also the description makes it sound like you're adding support for the feature rather than fixing the terrible error diagnostics - or at least to my early morning brain. :)

Also, one inline comment so far.

Thanks!

-eric

Caught me just in time :)

Sure, I can split this into a couple of patches. I'll abandon this one and go from there.

erichkeane abandoned this revision.Jul 18 2017, 8:54 AM

Going to split this into a few patches as Eric requested.