craig.topper added inline comments.
================
Comment at: include/clang/Basic/BuiltinsX86.def:642
+// SHSTK
+TARGET_BUILTIN(__builtin_ia32_incsspd, "vUi","u","shstk")
+TARGET_BUILTIN(__builtin_ia32_rdsspd, "UiUi","Un","shstk")
----------------
Space after commas to match the rest of the file
================
Comment at: include/clang/Driver/Options.td:1801
def mno_stackrealign : Flag<["-"], "mno-stackrealign">, Group<m_Group>;
+def mno_cet : Flag<["-"], "mno-cet">, Group<m_x86_Features_Group>;
+def mno_shstk : Flag<["-"], "mno-shstk">, Group<m_x86_Features_Group>;
----------------
erichkeane wrote:
> Since CET is a union of the other two, how do we handle conflicting
> configurations? Is -mcet -mno-ibt an error, or the same as mno-shstk?
> Repeat question for all combinations.
All the flags should go in the "X86 feature flags" section later in this file.
================
Comment at: lib/Basic/Targets/X86.cpp:619
Features["xsave"] = true;
+ } else if (Name == "cet") {
+ if (Enabled)
----------------
It looks like "cet" is meant to be a alias for two features? Should you avoid
putting it in Features map at line 545 lke we do with "sse4"? Then it will
never get sent to the backend and you can remove FeatureCET from X86.td
================
Comment at: lib/Basic/Targets/X86.cpp:620
+ } else if (Name == "cet") {
+ if (Enabled)
+ Features["shstk"] = Features["ibt"] = true;
----------------
Is -mno-cet intended to disable both features? Cause that doesn't happen with
the way this is written.
================
Comment at: lib/Basic/Targets/X86.cpp:687
HasMPX = true;
+ } else if (Feature == "+cet") {
+ HasSHSTK = true;
----------------
This shouldn't be needed. "+cet" implies +shstk and +ibt. Those should be added
to feature map before we call this method
================
Comment at: lib/Basic/Targets/X86.cpp:1230
.Case("mpx", HasMPX)
+ .Case("cet", HasSHSTK && HasIBT)
+ .Case("shstk", HasSHSTK)
----------------
I'm not sure what hasFeature is really used for, but I doubt "cet" needs to be
in it.
================
Comment at: test/CodeGen/builtins-x86.c:260
+ __builtin_ia32_incsspd(tmp_Ui);
+ __builtin_ia32_incsspq(tmp_ULLi);
----------------
I don't think you need to update this test. The intrinsic header test should be
enough. I don't know why this test exists.
================
Comment at: test/Driver/x86-target-features.c:75
+// RUN: %clang -target i386-unknown-linux-gnu -march=i386 -mno-cet %s -### -o
%t.o 2>&1 | FileCheck -check-prefix=NO-CET %s
+// CET: "-target-feature" "+cet"
+// NO-CET: "-target-feature" "-cet"
----------------
Should this check that +shstk and +ibt got added. That seems like the more
important thing to check.
Repository:
rL LLVM
https://reviews.llvm.org/D40224
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
