Page MenuHomePhabricator

[llvm-objdump] Suppress spurious warnings when parsing Mach-O binaries.
ClosedPublic

Authored by mtrent on Tue, Jan 28, 2:37 PM.

Details

Summary

llvm-objdump started warning when asked to disassemble a section that
isn't present in the input files, in Yuanfang Chen's change:
d16c162c9453db855503134fe29ae4a3c0bec936. The problem is that the
logic was restricted only to the generic llvm-objdump parser, not to the
Mach-O-specific parser used for Apple toolchain compatibility. The
solution is to log section names from the Mach-O parser.

The macho-cstring-dump.test has been updated to fail if it encounters
this new warning in the future.

Diff Detail

Event Timeline

mtrent created this revision.Tue, Jan 28, 2:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Jan 28, 2:37 PM
grimar added inline comments.Wed, Jan 29, 12:46 AM
llvm/test/tools/llvm-objdump/X86/macho-cstring-dump.test
5

I'd suggest using --implicit-check-not="warning:" FileCheck option instead. Testing the whole message is too brittle approach I think.

llvm/tools/llvm-objdump/MachODump.cpp
1753

Why this line was changed?

grimar added a comment.EditedWed, Jan 29, 12:48 AM

Also we usually add a corresponding tag to the patch name:
"Suppress spurious warnings when parsing Mach-O bianries." -> "[llvm-readobj] - Suppress spurious warnings when parsing Mach-O bianries."

+ "bianries" -> "binaries"

jhenderson added inline comments.Wed, Jan 29, 12:51 AM
llvm/test/tools/llvm-objdump/X86/macho-cstring-dump.test
5

+1. It also means you only need to specify the pattern in one place, rather than the current two.

mtrent marked 2 inline comments as done.Wed, Jan 29, 10:10 AM
mtrent added inline comments.
llvm/test/tools/llvm-objdump/X86/macho-cstring-dump.test
5

Sounds good, I'll update the patch.

llvm/tools/llvm-objdump/MachODump.cpp
1753

Looks like a remnant from an earlier attempt. Probably not needed.

mtrent updated this revision to Diff 241230.Wed, Jan 29, 11:09 AM

Review feedback: use --implicit-check-not to catch all unexpected warnings.
Remove unnecessary initialization.

mtrent marked 3 inline comments as done.Wed, Jan 29, 11:10 AM

LG, but I'll defer to @jhenderson and @grimar to approve.

MachODump.cpp needs more refactorings and probably even a rewrite of some parts. @mtrent If you know someone interested in this tool, please let them know:)

MaskRay retitled this revision from Suppress spurious warnings when parsing Mach-O bianries. to [llvm-objdump] Suppress spurious warnings when parsing Mach-O bianries..Wed, Jan 29, 11:29 AM
mtrent retitled this revision from [llvm-objdump] Suppress spurious warnings when parsing Mach-O bianries. to [llvm-objdump] Suppress spurious warnings when parsing Mach-O binaries..Wed, Jan 29, 1:01 PM
grimar accepted this revision.Thu, Jan 30, 12:37 AM

LGTM. Please also wait for @jhenderson approve.

This revision is now accepted and ready to land.Thu, Jan 30, 12:37 AM
This revision was automatically updated to reflect the committed changes.