Limit compressed debug to little-endian targets
ClosedPublic
Actions

Authored by emaste on Aug 7 2021, 2:49 PM.

Details

Reviewers

dim
jrtc27
jhb

Commits

rG47363e99d3d3: Enable compressed debug on little-endian targets

Summary

Older versions of LLD fail with big-endian compressed debug sections.
This was fixed in LLD upstream by c6ebc651b6fa and merged to FreeBSD
main in d69d07569ee2 by dim, but external toolchains will not yet have
the fix.  Limit to little-endian targets for now.

PR:             257638
Sponsored by:   The FreeBSD Foundation

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

emaste created this revision.Aug 7 2021, 2:49 PM

Herald added a subscriber: bdrewery. · View Herald TranscriptAug 7 2021, 2:49 PM

emaste requested review of this revision.Aug 7 2021, 2:49 PM

jrtc27 added inline comments.Aug 7 2021, 2:55 PM

share/mk/bsd.compiler.mk
244	Hm, I feel like this should be in both COMPILER_FEATURES and LINKER_FEATURES, and both must support it. Then you can leave this as-is and guard the LINKER_FEATURES by (LE \|\| !lld \|\| version > ...)?

Are there implications here for ports that link against static system libraries?

Are there implications here for ports that link against static system libraries?

AFAIK all non-obsolete GNU toolchains support compressed debug.

Perhaps this is an argument for leaving this disabled on BE (including in .o and .a) for the time being, as BE targets will continue to use llvm packages with broken lld for some time.

emaste added inline comments.Aug 7 2021, 7:34 PM

share/mk/bsd.compiler.mk
244	That's correct, although I'm not sure it's worth the added complexity in light of your point that having compressed debug in base system .a archives may pose a problem even if the base system linker is fixed. My preference is to go with this not-completely-correct change for now and just turn it on unconditionally once lld 13 is widely deployed.

imp added a subscriber: imp.Aug 7 2021, 10:23 PM

imp added inline comments.

share/mk/bsd.compiler.mk
244	Do you need a generic kill switch for this? Otherwise it looks good to me. I agreed with both the pedantry and the belief that making it more complicated wouldn't buy much in terms of added functionality to 'pay' for that 'complexity'.

emaste added inline comments.Aug 7 2021, 11:34 PM

share/mk/bsd.compiler.mk
244	Are you suggesting something like `WITH_`/`WITHOUT_COMPRESSED_DEBUG`? I'm not really sure it's necessary, and setting `DEBUG_FILES_CFLAGS?= -g` should work to disable it if desired. A `WITH_`/`WITHOUT_` build knob would probably entail including src.opts.mk into bsd.sys.mk (where `DEBUG_FILES_CFLAGS` is set).

imp added inline comments.Aug 7 2021, 11:39 PM

share/mk/bsd.compiler.mk
244	Including src.opt.mk anywhere in the bsd.*.mk files is likely bad... which was why I was asking if we needed one or not... I'm thinking not since there's ways to work around this, as you say, even if they aren't the most convenient... and the need would likely be niche enough to not give enough benefit to having a full WITHOUT_foo flag.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 8 2021, 5:36 PM

Closed by commit rG47363e99d3d3: Enable compressed debug on little-endian targets (authored by emaste). · Explain Why

This revision was automatically updated to reflect the committed changes.

emaste added a commit: rG47363e99d3d3: Enable compressed debug on little-endian targets.

bdrewery added inline comments.Aug 18 2021, 9:01 PM

share/mk/bsd.compiler.mk
238–244	Shouldn't all of these COMPILER_FEATURES depend on a compiler version? If I use an external clang is it guaranteed to have retpoline, init-all, and compressed-debug? I might be forgetting or missing something here that avoids external toolchains getting to this point.

jrtc27 added inline comments.Aug 18 2021, 9:10 PM

share/mk/bsd.compiler.mk
238–244	retpoline was in 7.0 and backported to 6.0 and 5.0. init-all looks like it was added for 8.0 (and by default is unused, you need one of the WITH_INIT_ALL_* options enabled). compressed-debug looks like it was added around 3.5. So technically yes there are missing version checks, but in practice there's basically no use case for many-years-old versions of Clang being used to build FreeBSD, especially when some of them, like retpoline, are annoying to check for and also really should just be required these days.

bdrewery added inline comments.Aug 18 2021, 11:46 PM

share/mk/bsd.compiler.mk
238–244	True. Works for me.

Yeah, for the ones I introduced the feature was already available in all relevant Clang versions so did not bother with a version check. Assuming this is now also true for GCC we might want to simplify things by removing the test and special cases.

In D31454#712703, @emaste wrote:

Yeah, for the ones I introduced the feature was already available in all relevant Clang versions so did not bother with a version check. Assuming this is now also true for GCC we might want to simplify things by removing the test and special cases.

We have enough special cases that would be great!

I've been slogging through sys/cdefs.h with this in mind, but since it's included by everything, it kinda begs the question of how far back do we, as a project, really support when it's just a compiler that's used....