This is an archive of the discontinued LLVM Phabricator instance.

[llvm-libtool-darwin] Add -warnings_as_errors
ClosedPublic

Authored by keith on Feb 3 2022, 11:41 AM.

Details

Summary

libtool can currently produce 2 warnings:

  1. No symbols were in the object file
  2. An object file with the same basename was specified multiple times

The first warning here is often harmless and may just mean you have some
translation units with no symbols for the target you're building for.
The second warning can lead to real issues like those mentioned in
https://reviews.llvm.org/D113130 where ODR violations can slip in.

This introduces a new -warnings_as_errors flag that can be used by build
systems that want to verify they never hit these warnings. For example
with bazel the libtool caller first uniques names to make sure the
duplicate base name case is not possible, but if that doesn't work as
expected, having it fail would be preferred.

It's also worth noting that llvm-libtool-darwin works around an issue
that cctools libtool experiences related to debug info and duplicate
basenames, the workaround is described here:
https://github.com/llvm/llvm-project/blob/30baa5d2a450d5e302d8cba3fc7a26a59d4b7ae1/llvm/lib/Object/ArchiveWriter.cpp#L424-L465
And it avoids this bug:
https://github.com/keith/radars/tree/f0cbbb1c37126ec6528c132510b29e08566377a7/DuplicateBasenameIssue

Diff Detail

Event Timeline

keith created this revision.Feb 3 2022, 11:41 AM
keith requested review of this revision.Feb 3 2022, 11:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2022, 11:41 AM
smeenai accepted this revision.Feb 3 2022, 1:15 PM

Ideally we'd have a way to automatically turn warnings into errors past a certain threshold (similar to how LLD's error handler does it), but given that we only have two warnings right now, that's probably overkill. This LGTM.

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
379

Would creating this as an Error and passing it to WithColor::defaultWarningHandler (as is done in the other case) work here as well, just to reduce the duplication?

This revision is now accepted and ready to land.Feb 3 2022, 1:15 PM
keith updated this revision to Diff 405781.Feb 3 2022, 2:04 PM
keith marked an inline comment as done.

Reduce duplication in warning / error string

jhenderson requested changes to this revision.Feb 4 2022, 12:33 AM

Please add the new option to the command guide documentation.

This revision now requires changes to proceed.Feb 4 2022, 12:33 AM
keith updated this revision to Diff 406041.Feb 4 2022, 10:59 AM

Add -warnings_as_errors to docs

keith updated this revision to Diff 406045.Feb 4 2022, 11:01 AM

Remove stray :

This revision is now accepted and ready to land.Feb 7 2022, 1:15 AM
This revision was landed with ongoing or failed builds.Feb 7 2022, 2:40 PM
This revision was automatically updated to reflect the committed changes.