Page MenuHomePhabricator

[InstrProfiling] Delete linkage/visibility toggling for Windows
ClosedPublic

Authored by MaskRay on Fri, May 28, 11:43 PM.

Details

Summary

The linkage/visibility of __profn_* variables are derived
from the profiled functions.

extern_weak => linkonce
available_externally => linkonce_odr
internal => private
extern => private
_ => unchanged

The linkage/visibility of __profc_*/__profd_* variables are derived from
__profn_* with linkage/visibility wrestling for Windows.

The changes can be folded to the following without changing semantics.

if (TT.isOSBinFormatCOFF() && !NeedComdat) {
  Linkage = GlobalValue::InternalLinkage;
  Visibility = GlobalValue::DefaultVisibility;
}

That said, I think we can just delete the code block.

An extern/internal function will now use private __profc_*/__profd_*
variables, instead of internal ones. This saves some symbol table entries.

A non-comdat {linkonce,weak}_odr function will now use hidden external
__profc_*/__profd_* variables instead of internal ones. There is potential
object file size increase because such symbols need /INCLUDE: directives.
However such non-comdat functions are rare (note that non-comdat weak
definitions don't prevent duplicate definition error).

Diff Detail

Event Timeline

MaskRay created this revision.Fri, May 28, 11:43 PM
MaskRay requested review of this revision.Fri, May 28, 11:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, May 28, 11:43 PM
rnk added a comment.Sat, May 29, 8:07 AM

The main impact of this change seems to be that PGO data for non-comdat code is now private instead of internal. That's a good change: PGO data has really high object file size overhead, and LLD already discards PGO symbol names from the PDB because they are so large.

Non-comdat weak code mainly comes from uses of __attribute__((weak)), and this change will make corresponding PGO data weak as well. LLVM's implementation of weak linkage on COFF is really convoluted, so I'm not sure this is correct. Consider adding an integration test case for __attribute__((weak)) to the compiler-rt profile test suite to see if this works.

MaskRay updated this revision to Diff 348662.Sat, May 29, 3:01 PM

rebase (after some pre-committed test improvement)
add a attribute((weak)) test

Herald added a project: Restricted Project. · View Herald TranscriptSat, May 29, 3:01 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
MaskRay updated this revision to Diff 348673.Sat, May 29, 9:15 PM

improve test/profile tests

they are unrelated to this patch so i'll pre-commit them if this is accepted.

MaskRay added inline comments.
compiler-rt/test/profile/Windows/coverage-linkage-lld.cpp
10 ↗(On Diff #348673)

An issue unrelated to this patch:

discarded* in coverage-linkage.cpp are discarded.

However, the weak symbol weak has weird mangling .weak.?weak@@YAXXZ.default.?discarded0@@YAXXZ (how does it contain discarded0 in its name)? @mstorsjo

mstorsjo added inline comments.Sun, May 30, 2:48 PM
compiler-rt/test/profile/Windows/coverage-linkage-lld.cpp
10 ↗(On Diff #348673)

That's because COFF weak symbols need to point at a different, non-weak global symbol. If you have multiple object files providing a weak symbol (which is meant to be allowed, at least in mingw environments), the default symbols that they point at would collide. Therefore, the default symbols get an extra string to make them unique - it tries to add the name of a non-comdat extern symbol defined in the same object file. As long as that symbol isn't multiply defined (which would cause conflicts in itself), that should make the defaults for the weak symbols unique too.

See D75989 for this odd design (which mimics what GCC/binutils do for that case).

MaskRay marked an inline comment as done.Tue, Jun 1, 1:36 PM
MaskRay added inline comments.
compiler-rt/test/profile/Windows/coverage-linkage-lld.cpp
10 ↗(On Diff #348673)

Thanks for the explanation!

MaskRay updated this revision to Diff 349107.Tue, Jun 1, 2:59 PM
MaskRay marked an inline comment as done.

Add coverage-weak-lld.cpp runtime tests - they add coverage. This patch doesn't change the behavior so I will commit the tests separately.

MaskRay updated this revision to Diff 349133.Tue, Jun 1, 4:41 PM

improve coverage-weak-lld.cpp

MaskRay added inline comments.Tue, Jun 1, 4:43 PM
compiler-rt/test/profile/Windows/coverage-weak-lld.cpp
63

@rnk The behavior change of this patch is here: previously this is 0 (similar to the PROFILE1 case). It is now 2 (like ELF) because the counter is weak and resolves to the %t0.o copy.

I think this doesn't matter because (1) the new behavior matches ELF (2) weak overriding weak is a mingw extension, not supported by link.exe.

rnk accepted this revision.Wed, Jun 2, 1:22 PM

lgtm, thanks

This revision is now accepted and ready to land.Wed, Jun 2, 1:22 PM
vsk added a comment.Wed, Jun 2, 2:01 PM

Thanks for adding these excellent tests. Lgtm as well.

compiler-rt/test/profile/Linux/coverage-linkage.cpp
39 ↗(On Diff #349133)

Consider leaving a brief note explaining how __attribute__((noinline)) inline affects linkonce_odr -- this may look confusing to some readers.

MaskRay updated this revision to Diff 349381.Wed, Jun 2, 2:30 PM
MaskRay marked an inline comment as done.

rename inline_ to linkonce_odr - this should be clearer about its linkage.