This is an archive of the discontinued LLVM Phabricator instance.

Return an error when dsymutil might produce an invalid mach-o file.
ClosedPublic

Authored by clayborg on Mar 10 2022, 11:31 AM.

Details

Summary

64 bit mach-o files have sections that only have 32 bit file offsets. If dsymutil tries to produce an invalid mach-o file, then error out with a good error string.

Diff Detail

Event Timeline

clayborg created this revision.Mar 10 2022, 11:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2022, 11:31 AM
clayborg requested review of this revision.Mar 10 2022, 11:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2022, 11:31 AM
aprantl added inline comments.Mar 10 2022, 11:35 AM
llvm/tools/dsymutil/MachOUtils.cpp
325

what does error() return?

JDevlieghere accepted this revision.Mar 10 2022, 11:42 AM

A little bike shedding about the error but otherwise this LGTM

llvm/tools/dsymutil/MachOUtils.cpp
325
This revision is now accepted and ready to land.Mar 10 2022, 11:42 AM
JDevlieghere added inline comments.Mar 10 2022, 11:42 AM
llvm/tools/dsymutil/MachOUtils.cpp
325

It prints the error and returns false.

Yeah, the error reporting was doing this pattern everywhere else, and I didn't want to break the pattern, or switch all of dsymutil over the llvm::Error...

clayborg updated this revision to Diff 414498.Mar 10 2022, 2:12 PM

Remove old code that was incorrectly checking for a total 4GB byte size of the final produced file. This code didn't work as intended for universal mach-o files since it was ok for these files to be larger than 4GB, and it didn't work for skinny mach-o files since they can actually be larger than 4GB as long as no uint32_t file offset would exceed UINT32_MAX.

JDevlieghere added inline comments.Mar 10 2022, 4:03 PM
llvm/tools/dsymutil/dsymutil.cpp
732 ↗(On Diff #414498)

So instead of the check below we now want to check the fat binary. Something like this:

uint64_t offset = 0;
for(size_t i = 0; i < NeedsTempFiles.size() - 1; ++i) {
  ErrorOr<vfs::Status> stat =
        Options.LinkOpts.VFS->status(OutputLocationOrErr->DWARFFile);
  if(!stat)
    break;
  offset += stat->getSize();
  if (offset > UINT32_MAX) 
    WithColor::error() << "the fat binary has a slice with an offset exceeds 4GB and will produce an invalid mach-o file.";
        return EXIT_FAILURE;
}
JDevlieghere requested changes to this revision.Mar 10 2022, 4:03 PM

Requesting changes to detect issues in fat Mach-Os

This revision now requires changes to proceed.Mar 10 2022, 4:03 PM
clayborg updated this revision to Diff 414526.Mar 10 2022, 4:32 PM

Add size detection for universal files with a slice that starts beyond 4GB, and clean up the error message as requested.

JDevlieghere accepted this revision.Mar 11 2022, 9:28 AM

Thanks Greg. LGTM with "Mach-O" everywhere for consistency.

llvm/tools/dsymutil/dsymutil.cpp
758 ↗(On Diff #414526)
This revision is now accepted and ready to land.Mar 11 2022, 9:28 AM
This revision was landed with ongoing or failed builds.Mar 11 2022, 10:00 AM
This revision was automatically updated to reflect the committed changes.