Page MenuHomePhabricator

Add the ability to map DLL storage class to visibility
ClosedPublic

Authored by bd1976llvm on Oct 22 2020, 9:34 AM.

Details

Summary

For PlayStation we offer source code compatibility with Microsoft's dllimport/export annotations; however, our file format is based on ELF.

To support this we translate from DLL storage class to ELF visibility at the end of codegen in Clang.

Other toolchains have used similar strategies (e.g. see the documentation for this ARM toolchain - https://developer.arm.com/documentation/dui0530/i/migrating-from-rvct-v3-1-to-rvct-v4-0/changes-to-symbol-visibility-between-rvct-v3-1-and-rvct-v4-0).

This patch adds the ability to perform this translation. Options are provided to support customizing the mapping behaviour.

Diff Detail

Event Timeline

bd1976llvm created this revision.Oct 22 2020, 9:34 AM
bd1976llvm requested review of this revision.Oct 22 2020, 9:34 AM
rnk added a comment.Oct 22 2020, 10:19 AM

Seems reasonable. I considered suggesting that this be a module pass, but I don't think it's worth it.

clang/include/clang/Driver/Options.td
4292

I don't think there is any benefit in having two spellings for this flag, one for cc1 and one for the driver. I'm guessing you are copying some prior art here, but I don't think it's necessary. I'd standardize on the _EQ versions, and add the CC1Option flag to the _EQ flags above.

clang/lib/CodeGen/CodeGenModule.cpp
713

This method (Release) is really long, and handles a lot of conceptually unrelated things to do at the end of codegen. Let's try to shorten it. Please move the bulk of this logic out into its own helper. I think it just needs langopts and the module.

724

I'd suggest one loop over M.global_values() to simplify the code. It will also handle ifuncs, for better or worse.

clang/lib/Driver/ToolChains/Clang.cpp
5237

I think if you standardize on the spelling with =, then there's a shorter way to forward driver flags to cc1. I forget the details, though.

MaskRay added inline comments.Oct 22 2020, 10:34 AM
clang/lib/Driver/ToolChains/Clang.cpp
5237
if (A->getOption().matches(options::OPT_fvisibility_from_dllstorageclass_EQ))
  A->render(Args, CmdArgs);
MaskRay added inline comments.Oct 22 2020, 10:37 AM
clang/test/Driver/visibility-dllstorageclass.c
1

This is not needed since lib/Target/X86/ code is not used. For -###, usually this is not needed.

33

NOT patterns can easily get stale and not check anything if no positive option is used together.

Can the values "hidden"/"protected" be removed from the pattern?

39

-DAG is actually more difficult to update. Keep the options in order and use -SAME

bd1976llvm marked 8 inline comments as done.Oct 23 2020, 5:59 PM
bd1976llvm added inline comments.
clang/include/clang/Driver/Options.td
4292

Thanks - this much better than what I had.

clang/lib/CodeGen/CodeGenModule.cpp
713

Thanks.

724

Thanks - it's shorter and handling ifuncs is a consistency improvement.

clang/lib/Driver/ToolChains/Clang.cpp
5237

Thanks - great suggestions.

clang/test/Driver/visibility-dllstorageclass.c
1

Thanks - obvious now you have pointed this out.

33

They can - thanks!

39

Thanks!

bd1976llvm marked 7 inline comments as done.

addressed review comments and made some other improvements:

  • reduced options logic using helpers e.g. AddLastArg().
  • simplified and fixed PS4 options logic.
  • added diagnostic for conflicting with other visibility options and testing for this.
  • added testing for unused diagnostics for the -fvisbility-from-dllstorageclass dependent options e.g. for -fvisibility-dllexport if -fvisbility-from-dllstorageclass is not set.
  • hardened and tidied tests.
rnk accepted this revision.Oct 29 2020, 4:20 PM

I'm happy with this, but check that @MaskRay is satisfied with it before landing.

This revision is now accepted and ready to land.Oct 29 2020, 4:20 PM
MaskRay added inline comments.Oct 29 2020, 4:54 PM
clang/include/clang/Basic/LangOptions.def
309

Note, other comments are not capitalized.

clang/include/clang/Driver/Options.td
1960

DriverOption should be removed

bd1976llvm marked 2 inline comments as done.

Addressed the review comments and made some minor improvements.

I also removed the diagnostic that -fvisibility-from-dllstorageclass is incompatible with other visibility options. I'm sorry to request a change like this after people have said that they are happy with the patch. However, I realised that I haven't tested that diagnostic downstream and I found a case where we were setting a visibility option to work around a bug. Therefore, I would like to remove that error from this upstream patch and experiment with what works best for us w.r.t to incompatibility diagnostics downstream before adding them upstream at a later time.

bd1976llvm added inline comments.Oct 29 2020, 8:01 PM
clang/include/clang/Basic/LangOptions.def
309

I have uncapitalized - thanks!

clang/include/clang/Driver/Options.td
1960

Thanks.

MaskRay accepted this revision.Oct 29 2020, 9:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2020, 9:08 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript