This is an archive of the discontinued LLVM Phabricator instance.

Explicit approach to controlling symbol visibility
AbandonedPublic

Authored by serge-sans-paille on Aug 25 2021, 1:24 AM.

Details

Reviewers
tstellar
dblaikie
Summary

Setting -fvisibility=hidden when compiling Target libs has the advantage of
not being intrusive on the codebase, but it also sets the visibility of all
functions within header-only component like ADT. In the end, we endup with
some symbols with hidden visibility within llvm dylib (through the target libs),
and some with external visibility (through other libs). This paves the way for
subtle bugs like https://reviews.llvm.org/D101972

It's easier to reason on explicit, pin-point linkage as provided in this patch.
This also paves the way for a more visibility control in all components,
something -fvisibility=hidden doesn't help with.

Diff Detail

Event Timeline

serge-sans-paille requested review of this revision.Aug 25 2021, 1:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2021, 1:24 AM
grandinj added inline comments.
llvm/lib/Target/AArch64/GISel/AArch64CallLowering.h
83

looks like the search and replace accidentally hit comments like these

Don't modify the closing brace namespace comment

serge-sans-paille marked an inline comment as done.Aug 27 2021, 3:41 AM

Is there a strategy for downstreams so that we don't have a weird mishmash of symbol visibilities, where files we've added have the old rules applied but files present upstream have the new rules? I don't particularly want to be trawling through both our modified backends trying to find every .h and .cpp file we've added, and people with complete out-of-tree backends will have even more of a hard time.

llvm/lib/Target/AArch64/AArch64.h
96

This has lost the actual namespace name

Is there a strategy for downstreams so that we don't have a weird mishmash of symbol visibilities, where files we've added have the old rules applied but files present upstream have the new rules?

I'm going to drop that patch, it's too intrusive. the alternative approach is to flag some classes used as parameter of llvm::Any as externally visible, I'm going to follow that way.

serge-sans-paille abandoned this revision.Aug 30 2021, 7:29 AM