This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Implement -arch_errors_fatal
ClosedPublic

Authored by keith on Nov 2 2021, 10:38 PM.

Details

Reviewers
int3
gkm
Group Reviewers
Restricted Project
Commits
rG6629ec3ecc16: [lld-macho] Implement -arch_errors_fatal
Summary

By default with ld64, architecture mismatches are just warnings, then
this flag can be passed to make these fail. This matches that behavior.

Diff Detail

Event Timeline

keith created this revision.Nov 2 2021, 10:38 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a subscriber: dang. · View Herald Transcript
keith requested review of this revision.Nov 2 2021, 10:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2021, 10:38 PM
keith added inline comments.
lld/MachO/InputFiles.cpp
798

I'm not sure if the unconditional return here is the right behavior, or if in the warning case something else should be done here. I think this is right though

keith updated this revision to Diff 384346.Nov 2 2021, 10:59 PM

Fix format

int3 accepted this revision.Nov 3 2021, 7:52 AM

Thanks!

lld/MachO/InputFiles.cpp
793

could we factor out the arguments somehow? E.g. something like auto f = config->errorForArchMismatch ? error : warning? (not sure if that compiles though)

798

yeah we do the same thing for checkCompatibility below so the early return of an incompletely initialized InputFile should be fine

lld/test/MachO/invalid/incompatible-arch.s
7

this should be no_fatal_warnings_lld too, otherwise we'll get an error regardless of whether -arch_errors_fatal is passed

This revision is now accepted and ready to land.Nov 3 2021, 7:52 AM
keith updated this revision to Diff 384548.Nov 3 2021, 11:52 AM
keith marked 3 inline comments as done.

Improve test, deduplicate string

keith added inline comments.Nov 3 2021, 12:06 PM
lld/MachO/InputFiles.cpp
793

I couldn't get a version of assigning the functions to a variable to work due to reference to overloaded function could not be resolved, I might have been doing something wrong?

Either way for now I added a simple lambda to de-duplicate the string, lmk what you think.

Note the whole reason I didn't assign the string to a variable instead was it seems like Twine is prime to use-after-free issues so the string was garbled in the output, clang-tidy also warned about that.

int3 added inline comments.Nov 3 2021, 9:01 PM
lld/MachO/InputFiles.cpp
793

ah yeah I guess we need to use static_cast to indicate which overload we want. this should work:

auto msg = config->errorForArchMismatch
               ? static_cast<void (*)(const Twine &)>(error)
               : warn;
msg(toString(this) + " has architecture " + getArchitectureName(arch) +
    " which is incompatible with target architecture " +
    getArchitectureName(config->arch()));
keith updated this revision to Diff 384652.Nov 3 2021, 9:34 PM
keith marked an inline comment as done.

Improve logic

lld/MachO/InputFiles.cpp
793

Ah thanks, I tried wrapping the whole ternary in the static_cast, but this works

keith updated this revision to Diff 384653.Nov 3 2021, 9:36 PM

Remove whitespace

This revision was automatically updated to reflect the committed changes.