Test change looks good (address lookup rather than by name), but it makes the objdump change completely irrelevant to this now, so I'd revert the objdump change.

This still needed after D76739?

Thanks for this!

• formatting

• rebase

• review comments

• review comments

• rebase

• replace objdump with readelf

• review comments

• fix formatting from clang-format

• review comments

In D75536#1904145, @MaskRay wrote:

In D75536#1903914, @davidb wrote:

In D75536#1903727, @MaskRay wrote:

Do you have a plan to implement --unique=pattern (overrides output section descriptions)? It is different from --unique (only applies to orphan sections).

I don't have a plan to implement --unique=pattern, as the default is good enough for my needs. However I would consider getting back and doing it when I next have some free cycles.

The merging of these sections in the case of a partial link (-r) inhibits deadstripping.

Dead stripping is a Mach-O term. Non --unique does not inhibit --gc-sections (without -r).
Without --unique, some orphan sections may be unnecessarily kept (as a whole their "liveness" can be larger than the composing parts).

Note that -r --gc-sections is not implemented in lld.

Sorry, force of habit. PlayStation toolchains also use the term dead-stripping for ELF binaries. I did indeed mean gc-sections.

The current workflow involves multiple link stages, involving a partial link and then an executable link. It is the second link that performs -gc-sections. We would like to keep all functions in their own individual sections after the partial link.

I could update the test to demonstrate this if it is at all useful.

A test is not needed. Just make the test minimal (for example, as I suggested).

Please update SUMMARY by mentioning the --gc-sections scenario and delete the mention of deadstripping.

• review comments

In D75536#1903727, @MaskRay wrote:

Do you have a plan to implement --unique=pattern (overrides output section descriptions)? It is different from --unique (only applies to orphan sections).

rebase

Fix lint checks

This looks great, thanks Jason!

In D74824#1891171, @jhenderson wrote:

In D74824#1891127, @davidb wrote:

In D74824#1890859, @jhenderson wrote:

Looks like you've got changes related to D74823 mixed in here?

Yes I put a note in the original description mentioning that this has been split to a separate change.

It would be clearer if you made them as two separate diffs, with the one that needs to be second based on the first one, so that Phabricator only shows the differences related to this patch (in other words, each Phabricator review represents one git commit). You can also indicate the dependency by saying "Depends on DXXXXXX" in the patch description, and Phabricator will then link the two patches together.

rebase

In D74824#1890859, @jhenderson wrote:

Looks like you've got changes related to D74823 mixed in here?

In D74823#1889317, @jhenderson wrote:

It's good that we don't crash, but I'm wondering why not just implement relocation printing for XCOFF? Also, is the intention to not print relocations mixed with disassembly only (i.e. -r + -d/D etc), or also relocation printing without disassembly (i.e. -r on its own)? If the latter, there needs to be a test for that too.

In D74823#1883269, @MaskRay wrote:

I think XCOFF is a WIP, so many things can break. It may be fine not adding a warning.

• fix space issues

In D73793#1854135, @jhenderson wrote:

I have a vague and probably unreasonable concern that changing from the beahviour for a filename "<stdin>" could cause somebody some confusion somewhere, since it's non-standard. I do agree with changing the error message though (we do similar things already in other tools, IIRC). What are others' thoughts?

• review comments

In D69997#1751678, @thakis wrote:

I don't know if there's a list anywhere. ls llvm/lib/Target shows you all in tree targets, LLVM_ALL_TARGETS in cmake (see llvm-cs.pcc.me.uk) shows the on by default targets, so adding the difference to LLVM_EXPERIMENTAL_TARGETS_TO_BUILD will likely give you all targets.

In D69997#1751467, @thakis wrote:

This breaks oodles of AVR tests: http://45.33.8.238/linux/4114/step_10.txt

In D69997#1751467, @thakis wrote:

This breaks oodles of AVR tests: http://45.33.8.238/linux/4114/step_10.txt

• Merge branch 'master' into disassembly_hex

• Merge branch 'master' into disassembly_hex
• Fix test after branch update

• remove whitesspace

• Split disassemble test case. Fix formatting
• formatting

What is the major request?

• review comments

• formatting

I haven't conditionally zero extended it in the case of -r. I'm not sure consistency with gnu objdump is worth it in that case and as there is a difference with -r -d it makes the code look. bit ugly. But I'm open to change that if you really insist...

• Add tets for 64-bit limits checking

In D69997#1738440, @jhenderson wrote:

Thanks for the patch. I'm surprised this hasn't broken any LLD tests. Did you run those too?

Thank you for the quick response.

Thanks for the detailed explanation, mostly tally's up with what I've just read. One thing

In D66426#1639065, @davidb wrote:

In D66426#1637540, @MaskRay wrote:

Another problem is that .note.gnu.build-id is SHF_ALLOC and included in a PT_LOAD segment. When computing watermark, you probably don't want to include its contents. So you should compute build-id and watermark first before you write.

What target are you looking at? I thought generally note sections aren't allocated and are usually safe to remove. If they were included in a segment, then that would be a PT_NOTE segment that isn't nested in a PT_LOAD segment.

In D66426#1637540, @MaskRay wrote:

Another problem is that .note.gnu.build-id is SHF_ALLOC and included in a PT_LOAD segment. When computing watermark, you probably don't want to include its contents. So you should compute build-id and watermark first before you write.

Updating D34685: Mark a number of x86 only tests to require x86

I can't seem to do it through the Phabricator UI, or archanist which has just created a new differential... https://reviews.llvm.org/D35565

Updating D35565: Update REQUIRES to use x86

Sorry, Jon and I have been out of office the past week.

In D34685#796778, @ruiu wrote:

I don't know if I understand what you said correctly. I do not want to force anyone to enable the x86 target. If you disable x86, tests that use x86 as a "generic" target won't run, but it shouldn't be more than that. I'm still waiting for you to update this patch to address Rafael's comment.

A few more tests have been introduced like this. It may make more sense to restructure the tests so that they are separated by target.