In D109972#3013964, @drodriguez wrote:

@mtrent: I noticed that change around the time of Xcode 12 (thanks!). However, even if the headers were available, I don't think LLVM would accept it as a dependency. What we are trying to do here is similar, with a common piece of code that handles the generation of these parts of the MachO binary. In the future, if it needs to be modified for some reason, all the tools should enjoy the improvements after a recompilation, instead of chasing down every tool implementation and modifying them.

In D109972#3013610, @drodriguez wrote:

This seems to be what happens when using Xcode's own strip (which is not based on LLVM), and it is something that we wanted to replicate in order to keep being compatible.

In D89561#2340681, @smeenai wrote:

@mtrent, do you have the historical context for why ld64 adds a leading space to the string table?

Looks good to me!

In D79309#2016936, @joerg wrote:

ARM's data detection only works for unstripped binaries. Other architectures often don't even have such markers at all.

Thanks for finding this!

In D62070#2000628, @MaskRay wrote:

It seems that we have some parsing difficulty here:/ I tried to use llvm-nm -s today for --print-armap (GNU/FreeBSD nm) and I noticed that -s is reserved for Mach-O. https://www.unix.com/man-page/osx/1/nm/ says

-s segname sectname
List only those symbols in the section (segname,sectname).

This also appears to be the last use case of cl::multi_val:/ Does -s have a longer option for the SegSect functionality? After all, it has been broken so many years... (the only order which was supported was llvm-nm /bin/ls -s __TEXT __text before this patch).

Add a comment about the purpose of the test.

In D78032#1978566, @MaskRay wrote:

In D78032#1978339, @mtrent wrote:

In D78032#1978249, @MaskRay wrote:

Can you clarify whether the intention is:

• For MachOObjectFile::getArchTriple, we want to use a conservative arch flag, because it may be used by both producers and consumers. We don't want to impose an arch feature restriction for binaries created by the producers.
• For llvm-objdump and may be llvm-nm/llvm-size, the read-only consumers, we want to use the latest arch flag so that all features can be decoded correctly.

I don't understand the question?

Is it possible to make the (defaulting mcpu to "apple-latest") decision in a library, instead of in llvm-objdump and other individual tools?

In D78032#1978249, @MaskRay wrote:

Can you clarify whether the intention is:

• For MachOObjectFile::getArchTriple, we want to use a conservative arch flag, because it may be used by both producers and consumers. We don't want to impose an arch feature restriction for binaries created by the producers.
• For llvm-objdump and may be llvm-nm/llvm-size, the read-only consumers, we want to use the latest arch flag so that all features can be decoded correctly.

In D78032#1978299, @MaskRay wrote:

In GNU objdump, -m is another option which takes an argument.

-m machine
 --architecture=machine
     Specify the architecture to use when disassembling object files.  This can be useful when disassembling object files which do not
     describe architecture information, such as S-records.  You can list the available architectures with the -i option.

     If the target is an ARM architecture then this switch has an additional effect.  It restricts the disassembly to only those
     instructions supported by the architecture specified by machine.  If it is necessary to use this switch because the input file does not
     contain any architecture information, but it is also desired to disassemble all the instructions use -marm.

Would it be possible to remove the -m alias for --macho?

Review feedback: Test -no-leading-headers separately from -no-leading-addr.

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

I am building a patch that will correct the problem. Feel free to roll back and I'll repush tomorrow.

Very good, I will note this is "not to be fixed" and return the request to support.

pinging reviewers.

Ping.

Attempt to clarify STAB symbol restriction comment.

Move MachO casts out of work loops.

Do we know what form that fix will take? And does that affect this PR?

Apparently fasm, x64, Linux, (the "flat assembler") as accessible via "tio.run" will accept "l" suffix as alternate form of the r*b registers. Here's a dorky existence proof:

In D71046#1771729, @craig.topper wrote:

I'm not sure what to do here. I'd like to see at least some other widely used tool supporting this. I worry we'll end up in a situation years from now where other tools try to match clang for what seems to have started as quirk in Intel's documentation nearly 15 years ago.

Add tests for these alternate registers.

In D71046#1771414, @craig.topper wrote:

Do you have examples of other tools that accept this? I checked the GNU assembler and it didn't accept r8l

In D71046#1770098, @craig.topper wrote:

Test case?

Logic looks fine. I have some concerns around alignment terminology that I suggest you address before committing.

mechanical comment changes

Updating to use path::append to build the bundle contents string.

LGTM, visual inspection only.

In D64102#1567303, @anushabasana wrote:

@mtrent We have implemented create with the -fat64 flag to specify a fat64binary, similar to cctools lipo.
Do we still need this flag, or can we always create a fat64 binary?
Another option would be to have some sort of auto detection to create a fat64 binary when the offset of a section exceeds 32 bits.

I wonder why the Mach-O specific commands are unlike that of other utilities ... In any case, these changes look fine.

In D63341#1552257, @anushabasana wrote:

In D63341#1549652, @mtrent wrote:

Note: "llvm-lipo" does not work properly with certain Mach-O executables, such as those built with the -hideARM64 flag.

Fixed most of these issues, but for printing a list of knows architectures I am planning to make a change in a separate diff that returns a list of valid architectures, since I will no longer be modifying llvm-lipo code.

For the MachO executables built with -hideARM64 flag, I filed a bug for MachOUniversalBinary: https://bugs.llvm.org/show_bug.cgi?id=42343.

In D63606#1553087, @MaskRay wrote:

BTW, there is a manpage: llvm/docs/llvm-objdump.1. It'd be nice to check if it needs updates as well.

In D63341#1547398, @alexshap wrote:

i think that we should not "replicate cctools lipo error messages exactly", ours could be better / more convenient or informative where it makes sense.
But in this particular case (see the comments above) I agree, including the names into the error message would be good!
Regarding exit codes - yes, it's important (for many reasons) to make sure that our exit codes are correct.

Would be nice if "llvm-lipo -thin" printed a reasonable error message before dumping usage. Apple lipo will print "missing argument to -thin option"

I'll have a look Monday / early next week. Meanwhile, here are some quick thoughts.

Respectfully, I am not a good reviewer for ELF-specific file format changes. I assume you added me as a proxy for 'enderby', who touched some of the lines of this routine. Looking at the SVN history I believe Kevin Enderby's involvement was limited to promulgating Lang Hames' "Expected<>" as the preferred error handling idiom in libObject and friends. So, again, respectfully, neither Kevin nor I are what I would consider good, authoritative reviewers for ELF.

All this looks reasonable, but my grasp of Mach-O is not as complete as I would like, and I have some questions for you.

approving this revision in light of my notes inline. I still believe this command guide is going to need an editorial pass once all of the options are in. my main concern for the moment is that the information is accurate and complete.

Thanks for making this!

I had trouble getting recommonmark.parser to install on my system, so I hacked at the patch to get the html and groff formatters to run. Should not be relevant to this review.

I am not familiar with lldb or with the DebugView library, so I am pulling in Fred Riss for this review. Thanks!

I did not download or run this code, but I believe it does not correctly divine the Arch flag from a given Mach-O binary. This is straight-forward to fix.