[PATCH] D129346: [X86] [Linux build][Stack Protector] Support for -mstack-protector-guard-symbol

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers accepted this revision.
nickdesaulniers added a comment.
This revision is now accepted and ready to land.

Tested i386 linux kernel builds+boots with CONFIG_STACKPROTECTOR now selected with clang! :) Thanks for the patch.

Only minor suggestions, feel free to ignore before landing.

Please add an entry about this feature to `clang/docs/ReleaseNotes.rst` (and maybe `llvm/docs/ReleaseNotes.rst` for the MD node).



================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3234-3242
+static bool isValidSymbolName(StringRef S) {
+  if (S.empty())
+    return false;
+
+  if (std::isdigit(S[0]))
+    return false;
+
----------------
If this is based on other code in clang like somewhere in the lexer, consider adding a comment that this is a similar implementation to that method, we just need it earlier in the driver to verify symbolic command line options.


================
Comment at: clang/test/Driver/stack-protector-guard.c:46
+// CHECK-SYM: "-cc1" {{.*}}"-mstack-protector-guard-symbol=sym"
+// INVALID-SYM: error: invalid value {{.*}} in 'mstack-protector-guard-symbol=', expected one of: legal symbol name
 
----------------
The `expected one of:` part is odd, I wonder if there's a better diag to use here? Maybe `diag::err_drv_argument_only_allowed_with` and then still use `"legal symbol name"`?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:2825-2831
+        if (!GV) {
+          Type *Ty = Subtarget.is64Bit() ? Type::getInt64Ty(M->getContext())
+                                         : Type::getInt32Ty(M->getContext());
+          GV = new GlobalVariable(*M, Ty, false, GlobalValue::ExternalLinkage,
+                                  nullptr, GuardSymb, nullptr,
+                                  GlobalValue::NotThreadLocal, AddressSpace);
+        }
----------------
It looks like the test in llvm/test/CodeGen/X86/stack-protector-3.ll tests the true case here, when the GV does not yet exist. Consider adding a test for when it does.


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

https://reviews.llvm.org/D129346