aaron.ballman added inline comments.
================
Comment at: include/clang/CodeGen/CGFunctionInfo.h:519
+ /// Whether this function has nocf_check attribute.
+ unsigned NoCfCheck : 1;
+
----------------
oren_ben_simhon wrote:
> aaron.ballman wrote:
> > This is unfortunate -- it bumps the bit-field over 32 bits. Can the bit be
> > stolen from elsewhere?
> The field is orthogonal to the other fields moreover i think that double
> meaning of the same field will lead to future bugs. The class is not a
> compact packed structure, so i don't feel it worth the confusion.
The class packs its fields for space savings because it's used fairly
frequently; if we can keep the size from having to use another allocation unit,
that's better. I wasn't suggesting we make a bit have double meaning, I was
wondering if we could reduce the size of one of the other fields. For instance,
I'd be surprised if we need all 8 bits for calling conventions, so we might be
able to reduce that to a 7-bit field.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:1979-1980
+static void handleNoCfCheckAttr(Sema &S, Decl *D, const AttributeList &Attrs) {
+ if (!S.getLangOpts().CFProtectionBranch)
+ S.Diag(Attrs.getLoc(), diag::warn_nocf_check_attribute_ignored);
+ else
----------------
oren_ben_simhon wrote:
> aaron.ballman wrote:
> > Can you use the `LangOpts` field to specify this in Attr.td? Then you can
> > go back to the simple handler.
> When using LangOpts field in Attr.td, the diagnostic warning will not be
> descriptive as i use here (use -fcf-protection flag...).
That's true, and this code is fine for now. However, it does suggest that the
declarative handler could be improved to support this sort of thing -- the same
issue is present with *all* attributes gated on a language option.
================
Comment at: test/Sema/attr-nocf_check.cpp:1
+// RUN: %clang_cc1 -verify -fcf-protection=branch -target-feature +ibt
-fsyntax-only %s
+
----------------
oren_ben_simhon wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > For better test coverage, you can switch to the `[[gnu::nocf_check]]`
> > > spelling in this file and pass `-std=c++11`
> > This one also likely needs an explicit triple.
> Since it doesn't test the functionality of this specific attribute, I believe
> it is an overkill to switch to [[gnu::nocf_check]] spelling.
> Since it doesn't test the functionality of this specific attribute, I believe
> it is an overkill to switch to [[gnu::nocf_check]] spelling.
C++ attribute spellings have slightly different code paths than GNU attribute
spellings, so the test isn't overkill. Also, this is a C++ test file that
already uses C++11, so adding the C++11 spelling is an improvement over using
the GNU spelling (that's already covered with C tests).
It's not critical, but it's still a strict improvement.
Repository:
rL LLVM
https://reviews.llvm.org/D41880
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
