FYI, I'm going to be away for 2 and a half weeks from the end of work today, so won't have time to look at these if I don't get to them later today. I have no issues with other people reviewing them. You might want to add @grimar and @MaskRay to the reviews as they've been doing a lot of work in obj2yaml/yaml2obj recently.

In D68636#1699484, @grimar wrote:

In D68636#1699479, @jhenderson wrote:

Did you remember to run LLD tests? I'd expect to see changes there...

Sure. That will be a separate change, once we agree with LLVM side.

Did you remember to run LLD tests? I'd expect to see changes there...

Thanks, LGTM (modulo the Minidump-specific details).

LGTM.

Can't comment too much on the file format details, but I've made some more general comments.

LGTM.

Not going to review the code (I'll leave others to do that), but a quick reminder to please include the full context when uploading diffs.

LGTM, since it makes things more readable. Beyond that, is there any particular motivation for this?

LGTM.

LGTM.

LGTM, with one request. I think we need a GNU version of this test too, but that's probably a separate change.

LGTM. I'll leave @rupprecht to answer @justice_adams's comment.

One suggestion. Otherwise, looks good.

I'm pleasantly surprised by how simple that FileCheck change is! You also need to update the FileCheck documentation (see llvm/docs/CommandGuide/FileCheck.rst) for the new option.

LGTM, with one nit.

LGTM.

In D68249#1689097, @MaskRay wrote:

I would argue that this is indeed an invalid case, though objdump -p does not error.

http://www.sco.com/developers/gabi/latest/ch5.dynamic.html#dynamic_section

Except for the DT_NULL element at the end of the array, and the relative order of DT_NEEDED elements, entries may appear in any order. Tag values not appearing in the table are reserved.

I take it that there should be a DT_NULL element at the end, so this section cannot be empty. The if (Dyn.back().d_tag != ELF::DT_NULL) test below becomes slightly more complex: "dynamic section must end with a DT_NULL" to "non-empty dynamic section must end with a DT_NULL".

That said, -p emitting an error may be a bit inconvenient because -x otherwise does not emit any error.

Maybe we should test: !empty or SHT_NOBITS?

BTW, do you have an opinion on D67090?

I don't know about any Mach-O specific details, but everything else looks good to me. Best get somebody else with Mach-O expertise to confirm.

LGTM, with a couple of minor points.

LGTM, but I wouldn't mind a second opinion.

LGTM, with one suggestion.

In D68066#1687377, @justice_adams wrote:

@rupprecht A bit of a beginner to LLVM, but why add the suppression for DynamicRelocations when --disassemble is provided?

LGTM, with a couple of minor nits.

In D68146#1686536, @alexshap wrote:

Oh, this is painful.
I'm curious - what do you think about some other options, e.g. about extending FileCheck to add support for case insensitiveness,
but maybe smb else might have a better idea

Latest changes LGTM.

In D67998#1682945, @wolfgangp wrote:

Yes, good observation. There is also the question what to do if the sizes of 2 different entries with the same address don't match. This should probably elicit a warning.

I agree this should probably print a warning, and I think it then should do a best guess print of the stack sizes. I have three options, in rough order of preference for this:

1. Display the entries with symbols in the order they appear in the symbol table, i.e. if foo and bar appear in that order in the symbol table, with the same address, I'd print foo with the first stack size and bar with the second. This is probably a reasonable guess based on what the linker might have done (putting the symbols in order they are discovered, same as the stack sizes entries).
2. Display only the largest entry, but once per symbol with the same address. In practice, this is probably just as good. It doesn't really make sense for two symbols to have the same address but different stack sizes: that would imply that the same bit of code has different possible sizes, which doesn't really make sense.
3. Display the first entry size, for each symbol. This is probably the simplest.

LGTM, thanks!

LGTM, with two suggestions.

LGTM.

In D67962#1682178, @grimar wrote:

In D67962#1682098, @jhenderson wrote:

I'm beginning to think that stack-sizes.test needs breaking up into smaller tests...!

I would probably start from splitting all the warning/error testing to stack-sizes-err.test.
They seem to take more than half (and still do not test all of the possible messages).

I'm beginning to think that stack-sizes.test needs breaking up into smaller tests...! This change LGTM anyway.

The latest changes look good themselves, but should this extra issue you've uncovered have a direct yaml2obj test?

One issue that just occurred to me, is what happens if you still have the stack size entry in this case? Something like:

LGTM, with one minor comment.

One major question to answer is how should --gap-fill interact with the preserving of segment contents that llvm-objcopy follows when segment contents are not covered by existing sections (see llvm/test/tools/llvm-objcopy/preserve-segment-contents.test for an example of the behaviour). I'd be inclined to have --gap-fill overwrite the data in this case, in which case a test should be written to show that the old data is no longer written in this case (see below for more details).

LGTM.

Looks good to me, but as agreed, let's wait for the GNU objcopy discussion to reach a conclusion before committing. I've posted there expressing my view. FWIW, our proprietary equivalent to objcopy uses raw values for this, rather than powers of two stuff, so the behaviour proposed in this patch would make life easier for them if and when they migrate over.

LGTM, thanks!

Thinks for the patch! I haven't had time to review the meat of it yet, but will try to get back to it in the next couple of days. One quick question in the meantime: what's the motivation behind this? Is it to improve GNU compatibility?

I'm very happy to see this change, but I hesitate to diverge in behaviour from GNU's behaviour. I certainly prefer --set-section-alginment to set it to an arbitrary alignment though, so I think we need to lean on the GNU maintainers to change their end.

LGTM, with two minor suggestions.

LGTM.

Looks good, once @grimar's comments have been addressed.

LGTM.

Looks good, aside from a concern with the error case.

Thanks, LGTM.

LGTM.

Address review comments:

• Reword introduction to be a bit more concise, and more closely match the POSIX description.
• Remove redundant comment about there being differences to GNU.
• Change --radix=x example to use -t x.