This is an archive of the discontinued LLVM Phabricator instance.

[clang][AIX] add option -mdefault-visibility-export-mapping
ClosedPublic

Authored by daltenty on May 24 2022, 3:49 PM.

Details

Summary

The option -mdefault-visibility-export-mapping is created to allow
mapping default visibility to an explicit shared library export
(e.g. dllexport). Exactly how and if this is manifested is target
dependent (since it depends on how they map dllexport in the IR).

Three values are provided for the option:

  • none: the default and exisiting behavior without the option, no additional export linkage information is created.
  • explicit: add the export for entities with explict default visibility from the source, including RTTI
  • all: add the export for all entities with default visibility

This option is useful for targets which do not export symbols as part of
their usual default linkage behaviour (e.g. AIX), such targets
traditionally specified such information in external files (e.g. export
lists), but this mapping allows them to use the visibility information
typically used for this purpose on other (e.g. ELF) platforms.

Diff Detail

Event Timeline

daltenty created this revision.May 24 2022, 3:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2022, 3:49 PM
daltenty requested review of this revision.May 24 2022, 3:49 PM
MaskRay added inline comments.May 24 2022, 9:46 PM
clang/lib/Driver/ToolChains/Clang.cpp
5969

I believe this should be AIX specific.

You may use err_drv_unsupported_opt_for_target instead of adding a new diagnostic.

clang/test/CodeGen/mpublic-visibility-export-mapping.c
1 ↗(On Diff #431818)

Use an AIX triple.

You can add a linux triple test that this option gives a warning.

No need for another test file mpublic-visibility-export-mapping-ms.cpp

daltenty added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
5969

I believe this should be AIX specific.

There are definitely other target's which could make use of this, but I think it's fine to let them opt-in as you suggest. I'll update the guard.

clang/test/CodeGen/mpublic-visibility-export-mapping.c
1 ↗(On Diff #431818)

Sounds good. I'll leave the original error in as an assert though, in case this ever get's turned on for this ABI, as we've only handled the itanium cases.

daltenty updated this revision to Diff 432084.May 25 2022, 12:34 PM

Address review comments:

  • Remove the error on ms abi and replace it with an assert
    • Rename tests and adjust triple
    • Make the option only available on AIX

docs/ClangCommandLineReference.rst is generated from HelpText. Use UsersManual.rst if you want to provide additional information. Please ensure --help has sufficient information about the allowed values.

[clang] add option mdefault-visibility-export-mapping

Don't omit - in -mdefault-visibility-export-mapping.

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

Search render(Args, CmdArgs) in the file.

clang/test/CodeGen/mdefault-visibility-export-mapping.c
20

This checks that UNSPECIFIED has dllexport but doesn't check that the other command does not have dllexport.
I'd suggest you just check define void @func() for on configuration and define dllexport void @func() for the other, instead of leaving CHECKs spanning across multiple lines.

daltenty retitled this revision from [clang] add option mdefault-visibility-export-mapping to [clang] add option -mdefault-visibility-export-mapping.May 25 2022, 2:17 PM
daltenty edited the summary of this revision. (Show Details)
daltenty marked an inline comment as done.May 25 2022, 6:18 PM
daltenty added inline comments.
clang/test/CodeGen/mdefault-visibility-export-mapping.c
20

This checks that UNSPECIFIED has dllexport but doesn't check that the other command does not have dllexport.

The --implicit-check-not dllexport ensures that dllexport doesn't appear anywhere it's not explicitly expected in the test. Now that we're using a specific triple I guess we can simplify as you suggest since it will improve the readability of the test.

docs/ClangCommandLineReference.rst is generated from HelpText

Thanks for that tip. When does this usually get regenerated? I see it is already out of sync with some option changes.

daltenty updated this revision to Diff 432166.May 25 2022, 7:19 PM

Address review comments:

  • Simplify tests
  • Reorganize docs
daltenty updated this revision to Diff 432167.May 25 2022, 7:22 PM

Remove now extraneous header include

MaskRay accepted this revision.May 25 2022, 11:33 PM
MaskRay added inline comments.
clang/test/CodeGenCXX/mdefault-visibility-export-mapping-rtti.cpp
95

For the std::nullptr_t block, placing FUND-DEF together, FUND-HID together, etc may be clearer than interleaving.

clang/test/CodeGenCXX/mdefault-visibility-export-mapping.cpp
81

ditto elsewhere

This revision is now accepted and ready to land.May 25 2022, 11:33 PM
daltenty updated this revision to Diff 433106.May 31 2022, 9:32 AM
daltenty marked 2 inline comments as done.
daltenty retitled this revision from [clang] add option -mdefault-visibility-export-mapping to [clang][AIX] add option -mdefault-visibility-export-mapping.May 31 2022, 9:49 AM
daltenty updated this revision to Diff 433470.Jun 1 2022, 11:24 AM

Fix copy paste error in test introduce in last update

daltenty updated this revision to Diff 433513.EditedJun 1 2022, 1:18 PM

git-clang-format to make the precommit checks happy

Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2022, 3:07 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
nikic added a subscriber: nikic.Jun 2 2022, 1:01 AM

It looks like this causes a major compile-time regression in the clang frontend: http://llvm-compile-time-tracker.com/compare.php?from=6232a8f3d61e5856c17e7b314385e9ea8068cdc1&to=8c8a2679a20f621994fa904bcfc68775e7345edc&stat=instructions For example, tramp3d-v4 is up 5% in -O0 builds.

Not familiar with this area, but my first guess would be that getLinkageAndVisibility() is actually expensive and you're introducing a ton of calls to it?

It looks like this causes a major compile-time regression in the clang frontend: http://llvm-compile-time-tracker.com/compare.php?from=6232a8f3d61e5856c17e7b314385e9ea8068cdc1&to=8c8a2679a20f621994fa904bcfc68775e7345edc&stat=instructions For example, tramp3d-v4 is up 5% in -O0 builds.

Not familiar with this area, but my first guess would be that getLinkageAndVisibility() is actually expensive and you're introducing a ton of calls to it?

@nikic thanks for catching this! Looking into it now, I'll put in a revert if I can't get things sorted shortly

hans added a subscriber: hans.Jun 2 2022, 6:16 AM

We're hitting an assertion in Chromium due to this:

llvm/clang/lib/AST/Decl.cpp:1510: clang::LinkageInfo clang::LinkageComputer::getLVForDecl(const clang::NamedDecl *, clang::LVComputationKind): Assertion `D->getCachedLinkage() == LV.getLinkage()' failed.

See https://bugs.chromium.org/p/chromium/issues/detail?id=1331274#c2 for a reproducer.

I've reverted this in d42fe9aa84203a8f51b43a901d72fdc39eea69f7 in the meantime.

thakis added a subscriber: thakis.Jun 2 2022, 6:32 AM

When you reland this, please reland 4463bd0f89181234e0cef982e21de2e96038f873 at the same time. Else, this breaks tests in some build configurations.

MaskRay added inline comments.Jun 2 2022, 9:56 AM
clang/lib/CodeGen/CodeGenModule.cpp
1228

Restrict this to AIX/XCOFF.

Really other binary format users will not need this.

daltenty updated this revision to Diff 435609.Jun 9 2022, 10:50 AM
  • Don't try to export internal linkage RTTI types
  • Make shouldMapVisibilityToDLLExport inline and provide a short-circuit evaluation in the case where no mapping is in effect. This should hopefully solve the compile time issues and asserts we are seeing on other platforms, as if the option isn't in effect we don't try to do any additional linkage computations over what we would normally do. Tested via profiling building tramp3d-v4 and our calls to the linkage computer fall back to what we were getting before this patch.
daltenty marked an inline comment as done.Jun 9 2022, 10:55 AM
daltenty added inline comments.
clang/lib/CodeGen/CodeGenModule.cpp
1228

I believe this is now done, via us now doing an early check on the setting of the LangOpt (which will always be None on non-AIX). No need for an extra check on the binary format.