[windows] IRGen: add support for DLL Storage semantics #2080
Conversation
compnerd
force-pushed
the
dllstorage
branch
9 times, most recently
from
4fbccd9
to
f076567
Compare
compnerd
force-pushed
the
dllstorage
branch
4 times, most recently
from
2642e49
to
9637315
Compare
|
CC @rjmccall |
compnerd
changed the title
| auto fnTy = llvm::FunctionType::get(returnTy, argTys, /*vararg*/ false); | ||
| auto fn = llvm::Function::Create(fnTy, llvm::GlobalValue::PrivateLinkage, | ||
| llvm::Twine(name), IGM.getModule()); | ||
| // FIXME: should we be setting the visibility and the DLL storage as well? |
There was a problem hiding this comment.
No, private things aren't exported.
There was a problem hiding this comment.
Right, but should actually explicitly set the visibility to hidden and the DLL storage to Default (local only)?
There was a problem hiding this comment.
Hidden visibility isn't meaningful for internal / private entities, and I believe LLVM considers it a consistency error to have that combination. There's no need to make this explicit in the code.
|
The basic approach looks good, but please extrapolate from my comments so far. :) |
|
@rjmccall updated based on the extrapolation of your original comments. Please have another look! |
|
@swift-ci Please test |
|
@rjmccall post holiday ping :-). |
| auto fnTy = llvm::FunctionType::get(returnTy, argTys, /*vararg*/ false); | ||
| auto fn = llvm::Function::Create(fnTy, llvm::GlobalValue::PrivateLinkage, | ||
| llvm::Twine(name), IGM.getModule()); | ||
| fn->setAttributes(IGM.constructInitialAttributes()); | ||
|
|
There was a problem hiding this comment.
I don't actually dislike these whitespace changes, but please try to avoid changing code that you're not actually changing.
There was a problem hiding this comment.
This applies to a fair amount of this diff, really.
| IGM.Module.getFunctionList().insert(insertBefore->getIterator(), fn); | ||
| else | ||
| IGM.Module.getFunctionList().push_back(fn); | ||
|
|
There was a problem hiding this comment.
That was to have the visibility/calling convention together. If you would rather that they just be split up around the original position of this, I can do that.
|
Just some patch-cleanliness requests this time, thanks. |
Add initial support for modelling DLL Storage semantics for global values. This is needed to support the indirect addressing mechanism used on Windows.
|
Updated. Sorry, the patch is pretty old, and I was going through and inspecting all the sites that I found, so I had changed code around at one point. I believe that only the sites where there are changes should now be part of the commit. Thanks for the patience with this! |
|
LGTM, thanks! |
|
@swift-ci Please test and merge |
What's in this pull request?
Resolved bug number: (SR-)
Before merging this pull request to apple/swift repository:
Triggering Swift CI
The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:
Smoke Testing
Validation Testing
Note: Only members of the Apple organization can trigger swift-ci.
Add initial support for modelling DLL Storage semantics for global values. This
is needed to support the indirect addressing mechanism used on Windows.