Page MenuHomePhabricator

[Clang][-fvisibility-from-dllstorageclass] Set DSO Locality from final visibility
ClosedPublic

Authored by bd1976llvm on Nov 19 2020, 3:26 AM.

Details

Summary

Ensure that the DSO Locality of the globals in the IR is derived from their final visibility when using -fvisibility-from-dllstorageclass.

Diff Detail

Event Timeline

bd1976llvm requested review of this revision.Nov 19 2020, 3:26 AM
bd1976llvm created this revision.

Im not sure if renaming the check prefix is actually helpful, but sure. This does seem like somewhat unfortunate. For the dllexport case, you really want dso_local. define dso_local global and declare dso_local global are preferable in this case as that guarantees a local definition, which is always the case for an exported symbol. You definitely do not want declare extern dso_local global. A few of the changes here are actually changing the IRGen from the desired state, is that intentional?

Im not sure if renaming the check prefix is actually helpful, but sure. This does seem like somewhat unfortunate. For the dllexport case, you really want dso_local. define dso_local global and declare dso_local global are preferable in this case as that guarantees a local definition, which is always the case for an exported symbol. You definitely do not want declare extern dso_local global. A few of the changes here are actually changing the IRGen from the desired state, is that intentional?

Thanks for looking at this. Yes, overriding the IRGen pessimistically was my intention. -fvisibility-from-dllstorageclass is supposed to override the effects of visibility options and annotations - and those can have an effect on the DSO locality. I had also thought that anyone deploying this option is probably in a similar circumstance to us where they have a file format that deviates somewhat from the expected rules. Therefore, setting DSO locality pessimistically and fixing things up in the linker makes sense. Having said that I can also see an argument for leaving the DSO locality as it is. Do you think it would be better to put this behaviour on a switch? It can be off by default but the PS4 driver can set it on?

compnerd accepted this revision.Nov 19 2020, 3:43 PM

I'm not convinced that making this an option is valuable at this time. Right now, I don't think that this functionality is broadly adopted, and, I suppose that if your linker is clever enough to perform optimizations thoroughly enough, it wouldn't matter too much (modulo link time explosion). I would like to see this more accurately explained in the documentation though - CodeGenModule.cpp L423-L424 would be a good site for that. Beyond that, LGTM.

This revision is now accepted and ready to land.Nov 19 2020, 3:43 PM

Improved comment.

Fixed spelling errors.

rnk accepted this revision.Nov 23 2020, 12:44 PM

Looks good to me

Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2020, 4:33 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript