[PATCH] D79910: [x86][seses] Add clang flag; Use lvi-cfi with seses

[Zola Bridges via Phabricator via llvm-commits]

zbrid marked 2 inline comments as done.
zbrid added a comment.

Thanks for the ping, Scott. I'll update this so I can get it submitted soon.



================
Comment at: clang/lib/Driver/ToolChains/Arch/X86.cpp:200
+    if (!Args.hasArg(options::OPT_mno_lvi_cfi)) {
+      Features.push_back("+lvi-cfi");
+      LVIOpt = options::OPT_mlvi_cfi;
----------------
sconstab wrote:
> Would it be better to add `FeatureLVIControlFlowIntegrity` as a dependency for `FeatureSpeculativeExecutionSideEffectSuppression` in `llvm/lib/Target/X86/X86.td`?
Thanks for the tip! Yeah, I will update to do that, but it looks like that only ensures an error will be thrown if they aren't used together rather than ensuring one is enabled when the other is enabled. Am I misunderstanding?


================
Comment at: llvm/lib/Target/X86/X86SpeculativeExecutionSideEffectSuppression.cpp:90
+  const X86Subtarget &Subtarget = MF.getSubtarget<X86Subtarget>();
+  if (!Subtarget.useSpeculativeExecutionSideEffectSuppression() &&
+      !EnableSpeculativeExecutionSideEffectSuppression)
----------------
sconstab wrote:
> Is it really necessary to have the target feature and the CLI flag? If SESES is required for, say, a *.ll file, then `+seses` can always be added as a target feature.
I think there should be a way to turn on SESES without lvi-cfi. Similar to how there are flags to turn on SLH in various configurations. I'll see if I can lower the number of flags while still enabling that possibility.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79910/new/

https://reviews.llvm.org/D79910