This is an archive of the discontinued LLVM Phabricator instance.

[ARM][CMSE] Implement CMSE attributes
ClosedPublic

Authored by chill on Dec 6 2019, 9:49 AM.

Details

Summary

This patch adds CMSE attributes cmse_nonsecure_call and cmse_nonsecure_entry.
As usual, specification is available here: https://developer.arm.com/docs/ecm0359818/latest

Patch by Javed Absar, Bradley Smith, David Green, Momchil Velikov, possibly others.

Diff Detail

Event Timeline

chill created this revision.Dec 6 2019, 9:49 AM

You should also add tests in Sema to ensure the attribute appertains to the proper subjects, accepts no arguments, etc (and diagnoses otherwise).

clang/include/clang/Basic/Attr.td
155

Might as well use cast<> here because you already verified the type with the isa<>.

157

Same here.

966

Should these also be exposed via a double-square bracket attribute in both C and C++ mode? If so, under what vendor namespace?

969

No new undocumented attributes, please.

972

If this is intended to apply to types, it should be a TypeAttr instead of an InheritableAttr.

973

Same question here.

975

I don't think the ExpectedFunctionType bit is needed -- the diagnostic text should be pulled from the FunctionTypeDef declaration.

977

Same here.

clang/include/clang/Basic/DiagnosticSemaKinds.td
3122

Please add single quotes around the attribute name, 'cmse_nonsecure_entry'

3189

I don't think this change is necessary.

clang/include/clang/Sema/ParsedAttr.h
943 ↗(On Diff #232594)

I think this can also be removed.

clang/lib/Sema/SemaDeclAttr.cpp
1997

You can just pass in AL instead of calling getAttrName().

clang/lib/Sema/SemaExpr.cpp
8040–8044

Rather than do isa<> followed by cast<>, better to just use dyn_cast<> and check the results.

8048

Drop the spurious parens.

clang/lib/Sema/SemaType.cpp
7102

Missing full stop at the end of the comment

7104

Can drop the getAttrName() call and just pass in attr.

7109

Missing full stop at the end of the comment

7115

Same here with getAttrName(). You may need to use warn_attribute_wrong_decl_type_str as well.

7132

Missing full stop at the end of the comment

7134

Drop the getAttrName()

chill updated this revision to Diff 246398.Feb 25 2020, 2:45 AM

Phew, finally able to get back to this ...

chill marked 19 inline comments as done.Feb 25 2020, 2:49 AM
chill marked an inline comment as done and an inline comment as not done.Feb 26 2020, 11:08 AM

You should also add tests in Sema to ensure the attribute appertains to the proper subjects, accepts no arguments, etc (and diagnoses otherwise).

Have not forgotten about this one, will add tests.

clang/include/clang/Basic/Attr.td
966

We have not yet decided on such an addition to the ACLE specification.

chill updated this revision to Diff 246977.Feb 27 2020, 9:12 AM
aaron.ballman added inline comments.Feb 28 2020, 7:43 AM
clang/include/clang/AST/Type.h
3570–3573

You've gone over 12 with this change, but I'm not certain what changes are needed to Bits to support it (that should have 16 bits of space). Ideas? At the very least, the comment sounds like it's stale now.

clang/include/clang/Basic/DiagnosticDriverKinds.td
383

This is spelled ROPI elsewhere (like in err_drv_ropi_incompatible_with_cxx) -- which capitalization should we use to be consistent?

Also, I think these errors should be combined and use a %select to choose between the terms.

clang/lib/Sema/SemaDecl.cpp
8765

Can elide braces, and I'm pretty sure you can use castAs<> rather than getAs<> due to the preceding assert.

clang/lib/Sema/SemaDeclAttr.cpp
1996

I don't think this check is necessary -- the common attribute handler should already deal with it.

7165–7167

This looks incorrect -- that is a type attribute, not a declaration attribute, so the case should be removed entirely.

clang/lib/Sema/SemaExpr.cpp
8037–8039

const auto * in both cases.

clang/test/Sema/arm-cmse.c
21–22

Can remove some extra whitespace.

chill marked an inline comment as done.Mar 17 2020, 3:59 AM
chill added inline comments.
clang/include/clang/AST/Type.h
3570–3573

Yeah, I'll update the comment. From https://github.com/llvm/llvm-project/commit/ed72e29 it looks like
the comment was mechanically updated with each added bit, whereas it should probably spell:

// Feel free to rearrange or add bits, but if you go over 16,
// you'll need to adjust the Bits field below, and if you add bits, you'll need to
// adjust Type::FunctionTypeBitfields::ExtInfo as well
aaron.ballman added inline comments.Mar 17 2020, 5:31 AM
clang/include/clang/AST/Type.h
3570–3573

Agreed.

chill updated this revision to Diff 250839.Mar 17 2020, 11:23 AM
chill marked 8 inline comments as done.Mar 17 2020, 11:25 AM
aaron.ballman accepted this revision.Mar 20 2020, 11:36 AM

LGTM aside from some minor nits.

clang/include/clang/Basic/AttrDocs.td
4858–4859

I think we typically go with two space indentation in td files, so this (and below) looks a bit over-indented.

clang/lib/Sema/SemaDeclAttr.cpp
2001–2003

if (cast<FunctionDecl>(D)->getStorageClass() == SC_Static) { is shorter and has the same assertion behavior.

This revision is now accepted and ready to land.Mar 20 2020, 11:36 AM
chill updated this revision to Diff 252018.Mar 23 2020, 6:49 AM
chill edited the summary of this revision. (Show Details)
chill marked 2 inline comments as done.Mar 23 2020, 7:12 AM
chill updated this revision to Diff 252047.Mar 23 2020, 8:03 AM

Rebased, resolved some merge conflicts, fixed a few nits.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2020, 3:44 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
snidertm added inline comments.Apr 1 2020, 2:32 PM
clang/include/clang/AST/Type.h
3588

Shouldn't RegParmOffset be updated to 9, I believe it is used to shift the regParm value so that it encoded in the bits above CmseNSCallMask

chill marked 3 inline comments as done.Apr 1 2020, 2:56 PM
chill added inline comments.
clang/include/clang/AST/Type.h
3588

Hmm, I think 8 is OK, but we should mask it ...

3622

... here.

bool getHasRegParm() const { return ((Bits & RegParmMask) >> RegParmOffset) != 0;
snidertm added inline comments.Apr 1 2020, 3:34 PM
clang/include/clang/AST/Type.h
3604

Wouldn't this overwrite the CmseNSCallMask bit if hasRegParm is true and ((regParm + 1) & 1) is non-zero?

3622

I don't see how this helps. If RegParmOffset is 8 and the CmseNSCall bit is set in Bits, then your proposed getHasRegParm() will return true. Given the above assignment to Bits, we don't know if the CmseNSCall bit was set by cmseNSCall or by regParm.

MIght I be missing something?

chill marked 2 inline comments as done.Apr 1 2020, 4:36 PM
chill added inline comments.
clang/include/clang/AST/Type.h
3622

No, it will not return true, because the mask will clear all bits, except bits [8-10,13-31].
Bits [13-31] are unused/zero, and in the patch I'm preparing, the RegParmMask will be simply 0x700,
so they will be cleared anyway.
CmseNSCall is bit 12, so it will be cleared. Also RegParm + 1 is at most 7, so, it cannot overflow into
NoCfCheck of CmseNSCall.

chill marked an inline comment as done.Apr 1 2020, 4:49 PM
snidertm added inline comments.Apr 1 2020, 5:03 PM
clang/include/clang/AST/Type.h
3622

Got it, ok.

Have you already committed support for CMSE attributes to the LLVM backend? Or is that on the way?

snidertm added inline comments.Apr 1 2020, 7:43 PM
clang/lib/CodeGen/CGCall.cpp
1882

Just curious … Does the LLVM backend have a way to extract a StringRef attribute from a CallInst? I know that you can do that with hasFnAttribute(), but I don't see anything for CallInst objects

chill added a comment.Apr 2 2020, 1:35 AM

Have you already committed support for CMSE attributes to the LLVM backend? Or is that on the way?

The last two CMSE patches are under review: https://reviews.llvm.org/D76518

chill marked 4 inline comments as done.Apr 2 2020, 1:38 AM
chill added inline comments.
clang/lib/CodeGen/CGCall.cpp
1882
 CallInst *CI = ... ;
... = CI->getAttributes().hasFnAttribute("cmse_nonsecure_call"))