Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autodesk: Enable -fvisibility=hidden and -fvisibility-inlines-hidden to hide symbols on Linux and macOS #3452

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

erikaharrison-adsk
Copy link
Contributor

@erikaharrison-adsk erikaharrison-adsk commented Dec 3, 2024

Description of Change(s)

This change intends to enable building with -fvisibility=hidden to hide the symbols. for these benefits:

  1. It allows compiler and linker to do more optimizations, makes startup faster and binaries smaller
  2. Minimization of the set of exported symbols in order to track ABI changes
  3. Compliance with some specification or standard

Post on AOUSD: Support building with -fvisibility=hidden

General technical comment

We have added ARCH_EXPORT_TYPE to classes, structs, enums, and some instantiated templates that require visible symbols, to instruct the compiler to generate visible symbols.

On windows, the macro ARCH_EXPORT_TYPE is expanded to empty in MSVC because the current symbol export works fine and does not need to be modified. The reason for not replacing the member function export with a class-level export in this PR is to reduce the workload and to avoid cumbersome and complex technical constraints, which would require a lot of non-patterned modifications.

It's worth noting that visible and export are not equivalent terms, even though we may not be aware of the difference in our day-to-day work. It helps to understand some seemingly strange problems. For example, Clang can optimize and not to generate code and symbols for inline functions marked as visible, which can lead to symbol lookups failure when linking across modules.

Further cleanup can be done by following up with smaller PRs, which do not have to be as large as this one, so the effort and risk is more manageable.

In Clang, the visibility of an instantiated type of a template depends on both the template and template arguments.

template<typename T> class __attribute__((visibility("default"))) Foo{};
class __attribute__((visibility("hidden"))) Bar{};

the visibility of Foo<Bar> would be hidden other than default, even Foo is marked as default. That is why this PR added "default" to some enums and lots of simple struct types.

Link to proposal (if applicable)

  • N/A

Fixes Issue(s)

Related issues:

Checklist

@asluk asluk added build Build-related issue/PR needs review Issue needing input/review by the repo maintainer (Pixar) labels Dec 3, 2024
@jesschimein
Copy link
Collaborator

Filed as internal issue #USD-10497

@jesschimein
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AlexSchwank
Copy link
Contributor

AlexSchwank commented Dec 5, 2024

Have you tried this on arm64 macOS? This might break type comparison where -fvisibility=hidden sets a uniqueness bit. This is what the workaround here attempts to fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Build-related issue/PR needs review Issue needing input/review by the repo maintainer (Pixar)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants