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

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm added a comment.

Sorry for later response!
@nickdesaulniers Your suggestions are very make sense. 
I'll refine it soon. Thanks so much!



================
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;
+
----------------
nickdesaulniers wrote:
> 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.
The lexer is checking letter one by one and classify them immediately, not check all the symbol name is legal or not.
For example:

int 3zxzx;

```
t.c:1:5: error: expected identifier or '('
int 3zxzx;
    ^
1 error generated.
```


================
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
 
----------------
nickdesaulniers wrote:
> 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"`?
Good catch and guide, thanks!


================
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);
+        }
----------------
nickdesaulniers wrote:
> 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.
I think you want to add both **existed **"__woof" and **non-existed**  case here. Here lack the **existed ** one.
Ok, let me add it.


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

https://reviews.llvm.org/D129346