This is an archive of the discontinued LLVM Phabricator instance.

[IR] Don't allow DLL storage-class and local linkage
ClosedPublic

Authored by bd1976llvm on Sep 27 2022, 8:11 PM.

Details

Summary

Disallow this meaningless combination. Doing so simplifies analysis of LLVM code w.r.t t DLL storage-class, and prevents mistakes with DLL storage class.

  • Change the assembler to reject DLL storage class on symbols with local linkage.
  • Change the bitcode reader to clear the DLL Storage class when the linkage is local for auto-upgrading
  • Update LangRef.

There is an existing restriction on non-default visibility and local linkage which this is modelled on.

Diff Detail

Event Timeline

bd1976llvm created this revision.Sep 27 2022, 8:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2022, 8:11 PM
bd1976llvm requested review of this revision.Sep 27 2022, 8:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2022, 8:11 PM
MaskRay added inline comments.Sep 27 2022, 9:50 PM
clang/lib/CodeGen/CodeGenModule.cpp
6016–6022

Prevent local linkage globals from ... or just "Don't assign ... to local linkage ..."

6017

llvm::GlobalValue::isLocalLinkage(Linkage) ?

This change is to make CodeGenCXX/reference-temporary-ms.cpp work

llvm/include/llvm/IR/GlobalValue.h
279
bd1976llvm marked 3 inline comments as done.Sep 28 2022, 6:56 AM

Thanks for the review. I addressed your comments and also consolidated the tests into a single test using split-file.

clang/lib/CodeGen/CodeGenModule.cpp
6017

Thanks. Yes, CodeGenCXX/reference-temporary-ms.cpp will fail without this change. It also just makes sense that we don't need to resolve the properties of a local linkage GV.

bd1976llvm marked an inline comment as done.
MaskRay accepted this revision.Sep 28 2022, 9:54 AM

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.

llvm/lib/AsmParser/LLParser.cpp
1182

Should Storage be storage?

must not or cannot ?

llvm/lib/Bitcode/Reader/BitcodeReader.cpp
1301

cannot be used together with

or

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

3771

ditto

3774

ensure both then and else branches have braces

3944–3945

ditto

This revision is now accepted and ready to land.Sep 28 2022, 9:54 AM

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.

Can you give an example? MSVC certainly rejects simple cases e.g. https://godbolt.org/z/bG3M7oj5W. I am worried that I have missed something...

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.

Can you give an example? MSVC certainly rejects simple cases e.g. https://godbolt.org/z/bG3M7oj5W. I am worried that I have missed something...

__declspec(dllexport) const int foo = 10; foo with internal linkage is accepted.

rnk added a comment.Sep 29 2022, 9:39 AM

__declspec(dllexport) const int foo = 10; foo with internal linkage is accepted.

I believe in this case MSVC will make the symbol external, so it's not actually internal. The dll attributes tend to imply extern in many cases.

bd1976llvm marked 5 inline comments as done.Sep 29 2022, 4:15 PM

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.

Can you give an example? MSVC certainly rejects simple cases e.g. https://godbolt.org/z/bG3M7oj5W. I am worried that I have missed something...

__declspec(dllexport) const int foo = 10; foo with internal linkage is accepted.

Thanks. I have reviewed the discussion here: https://reviews.llvm.org/D45978 and I experimented with the MSVC toolchain.

__declspec(dllexport) const int foo = 10; foo with internal linkage is accepted.

I believe in this case MSVC will make the symbol external, so it's not actually internal. The dll attributes tend to imply extern in many cases.

Thanks. Interestingly, MinGW seems to reject such constructs which is also the behaviour that we implement for PlayStation.

In conclusion, as MaskRay alluded to upthread, this anomaly means that this restriction can't be applied during Sema. However, it does make sense for the IR.

Address final review comments.

bd1976llvm closed this revision.Sep 29 2022, 4:28 PM