[PATCH] D40224: [X86] Control-Flow Enforcement Technology - Shadow Stack and Indirect Branch Tracking support (Clang side)

[Craig Topper via Phabricator via cfe-commits]

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