This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Change -z unknown from error to warning
ClosedPublic

Authored by MaskRay on Nov 29 2021, 3:20 PM.

Details

Summary

There is a trend of having more optional options (usually security
hardening related) like -z cet-report=, -z bti-report=, -z force-bti.
If ld.lld 14.0.0 uses a warning, in 15/16/17/... timeframe when people
add new options to software, they can worry less about linker errors on ld.lld 14.0.0.

In some cases -z foo does essential work where a silent ignore can be
problematic, but the user has received a warning. From my observation, the
doing-essential-work -z foo is much fewer than the converse. In addition,
the user who cares can use --fatal-warnings (Note: GNU ld doesn't upgrade warnings to errors).
It is unclear whether we need something like clang -Wunknown-warning-option.

If we ever run into unfortunate transition like -z start-stop-gc, the
affected software (e.g. ldc is a compiler which passes linker options to the underlying ld)
can blindly add the -z option, without worrying it may cause a linker error to LLD 14.0.0.

Diff Detail

Event Timeline

MaskRay created this revision.Nov 29 2021, 3:20 PM
MaskRay requested review of this revision.Nov 29 2021, 3:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 29 2021, 3:20 PM
MaskRay edited the summary of this revision. (Show Details)Nov 29 2021, 3:21 PM
MaskRay edited the summary of this revision. (Show Details)

Probably worth pointing out this is what GNU ld does, so there's precedent (that's not just what Clang does)

lld/test/ELF/driver.test
66

Should there be --fatal-warnings versions of these? Might be overkill given it's just a call to warn but at the same time doesn't hurt to be extra safe.

MaskRay updated this revision to Diff 390498.Nov 29 2021, 3:34 PM

Move fatalWarnings and test it

MaskRay marked an inline comment as done.Nov 29 2021, 3:34 PM
MaskRay updated this revision to Diff 390499.Nov 29 2021, 3:35 PM

update a comment

jrtc27 accepted this revision.Nov 29 2021, 3:44 PM

But I suggest you get an approval from more than just me

This revision is now accepted and ready to land.Nov 29 2021, 3:44 PM
peter.smith accepted this revision.Nov 30 2021, 2:49 AM

LGTM too, good to match GNU ld behaviour where possible. One stale comment spotted but easy to fix up prior to commit.

lld/ELF/Driver.cpp
464

Comment is now stale as we're not reporting an error. Perhaps
// Report a warning for an unknown -z option to match ld.bfd behavior.

MaskRay updated this revision to Diff 390759.Nov 30 2021, 11:06 AM

Update stale comment

This revision was landed with ongoing or failed builds.Nov 30 2021, 11:06 AM
This revision was automatically updated to reflect the committed changes.
MaskRay marked an inline comment as done.Nov 30 2021, 11:07 AM