[PATCH] D134784: [IR] Don't allow DLL storage-class and local linkage

[Fangrui Song via Phabricator via llvm-commits]

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

LGTM.
For LLVM IR, the more rigid form helps.
For Clang, it seems that MSVC allows dllspec on a local linkage variable, so that's why we have to add it to CodeGen instead of rejecting it in Sema.



================
Comment at: llvm/lib/AsmParser/LLParser.cpp:1182
+    return error(NameLoc,
+                 "symbol with local linkage must not have a DLL Storage class");
+
----------------
Should `Storage` be `storage`?

`must not` or cannot ?


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:1301
 static void upgradeDLLImportExportLinkage(GlobalValue *GV, unsigned Val) {
+  // Local linkage must not have a DLL Storage class.
+  if (GV->hasLocalLinkage())
----------------
cannot be used together with

or

A GlobalValue of a local linkage cannot have a ...


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:3771
+  if (Record.size() > 10) {
+    // Local linkage should not have a DLL Storage class.
+    if (!NewGV->hasLocalLinkage())
----------------
ditto


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:3774
+      NewGV->setDLLStorageClass(getDecodedDLLStorageClass(Record[10]));
+  } else
     upgradeDLLImportExportLinkage(NewGV, RawLinkage);
----------------
ensure both `then` and `else` branches have braces


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:3941
+  } else
     upgradeDLLImportExportLinkage(Func, RawLinkage);
 
----------------
ditto


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

https://reviews.llvm.org/D134784