This is an archive of the discontinued LLVM Phabricator instance.

[profile][test] Add -fuse-ld=bfd to make instrprof-lto-pgogen.c robust
ClosedPublic

Authored by MaskRay on Jul 19 2020, 7:42 PM.

Details

Summary

Otherwise if 'ld' is an older system LLD (FreeBSD; or if someone adds 'ld' to
point to an LLD from a different installation) which does not support the
current ModuleSummaryIndex::BitCodeSummaryVersion, the test will fail.

Add lit feature 'binutils_lto'. GNU ld is more common than GNU gold, so
we can just require 'is_binutils_lto_supported' to additionally support GNU ld.

Diff Detail

Event Timeline

MaskRay created this revision.Jul 19 2020, 7:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2020, 7:42 PM
Herald added subscribers: Restricted Project, dexonsmith, steven_wu and 5 others. · View Herald Transcript
myhsu added a comment.Jul 20 2020, 9:09 AM

According your problem statement, I think the correct way to approach it is to fix the environment (i.e. use the correct version of LLD) because apparently the root of cause is outdated linkers. In other words, your original argument can be extrapolated to outdated GNU as, outdated objdump, or even outdated shell. But I don't think it's a good idea to put version constrains on every of these items, or say "because it's possible that users are using an old version of bash shell, so let's use sh shell"

Also, I'm not sure if this test will work on platform that doesn't have BFD linker at all (e.g. Mac OSX). Although my original patch was fixing a problem targeting BFD linkers, but the original test (i.e. the one without -fuse-ld=bfd) will run on platforms (MSVC will fail but it's not caused by not finding a proper linker) regardless of what the system linker is, because the driver will handle that. But I'm worry that if you explicitly assign the linker to use, platforms without BFD linker will unable to fulfill your linker requirement and fail at the very early stage.

According your problem statement, I think the correct way to approach it is to fix the environment (i.e. use the correct version of LLD) because apparently the root of cause is outdated linkers.

I don't agree with your statement. If you look at various UNSUPPORTED: XFAIL: changes to compiler-rt/test/, many changes are due to reasonable environment differences. On FreeBSD, /usr/bin/ld is a system LLD of an older version.

In other words, your original argument can be extrapolated to outdated GNU as, outdated objdump, or even outdated shell. But I don't think it's a good idea to put version constrains on every of these items, or say "because it's possible that users are using an old version of bash shell, so let's use sh shell"

That is why we can't use objdump, GNU as, readelf, etc. Instead, we use llvm-objdump, llvm-mc and llvm-readelf. I picked -fuse-ld=bfd because I saw -fuse-ld=gold which is already used in a few tests.

Also, I'm not sure if this test will work on platform that doesn't have BFD linker at all (e.g. Mac OSX). Although my original patch was fixing a problem targeting BFD linkers, but the original test (i.e. the one without -fuse-ld=bfd) will run on platforms (MSVC will fail but it's not caused by not finding a proper linker) regardless of what the system linker is, because the driver will handle that. But I'm worry that if you explicitly assign the linker to use, platforms without BFD linker will unable to fulfill your linker requirement and fail at the very early stage.

That was actually my original complaint. The test should be restricted to a few platforms where it makes sense.

MaskRay updated this revision to Diff 279419.Jul 20 2020, 9:24 PM

REQUIRES linux

arichardson added inline comments.Jul 20 2020, 11:33 PM
compiler-rt/test/profile/instrprof-lto-pgogen.c
1

I wonder if this should be using a new REQUIRES: ld.bfd feature instead of assuming Linux means fuse-ld=bfd will work?

myhsu added inline comments.Jul 21 2020, 8:24 AM
compiler-rt/test/profile/instrprof-lto-pgogen.c
1

I think that's a good idea, it can solve many debates we discussed here

MaskRay updated this revision to Diff 279583.Jul 21 2020, 11:00 AM
MaskRay edited the summary of this revision. (Show Details)

Add binutils_lto

MaskRay marked 2 inline comments as done.Jul 21 2020, 11:01 AM
myhsu added inline comments.Jul 21 2020, 8:42 PM
compiler-rt/test/lit.common.cfg.py
447

I think this line is redundant (duplicate with line 461)

MaskRay updated this revision to Diff 279702.Jul 21 2020, 9:29 PM
MaskRay marked an inline comment as done.

Delete config.available_features.add('lto')

myhsu accepted this revision.Jul 22 2020, 9:06 AM

Thank you!
LGTM

This revision is now accepted and ready to land.Jul 22 2020, 9:06 AM
MaskRay updated this revision to Diff 279873.Jul 22 2020, 10:15 AM

Fix a typo

This revision was automatically updated to reflect the committed changes.