Thanks for fixing this Ben, once James' suggestions are fixed it LGTM.

@urnathan I've created the ticket https://github.com/llvm/llvm-project/issues/55171 regarding the mangling issue. Just to note here, above I mentioned that I thought the mangled output matched GCC but this is not the case.

Updated with urnathan's suggestion.

I now see what you mean @dblaikie . I believe the mangling is functioning correctly and it's output matches GCC, it is the demangling that is the issue. By following @urnathan's code suggestion the demangled output will differ but I don't think this should be a problem as they are equivalent?

This looks like a good change to me, just needs that ranlib test fixing.

Apologies for the delay in response and thank you for your input @dblaikie and @urnathan.

Move testing to area suggested by @urnathan.

Fixed failing test and @jhenderson's other points.

I am not so familiar with this part of LLVM, I think this testing may belong elsewhere but I am unsure where that would be.

Updated after https://reviews.llvm.org/D123778 was accepted and committed.

Good idea @MaskRay, I've split that into https://reviews.llvm.org/D123778 and will update this one when D123778 is complete.

Corrected comments including replacing use of "full archive" with "regular archive" as suggested. To remain consistent two tests have been renamed to use the term "regular". The checks for "thinness" in llvm-ar.cpp now only applies for write commands.

The following tests have been modified:

• flatten-thin-archive.test now explicitly checks that %t.a is thin after the other archives have been added.
• full-to-thin-archive.test uses Inputs/a.txt as an input rather than itself as spurious passes could have occurred when looking for strings contained in the archive file. Corrected a comment, added a test case for an existing archive and a case attempting to convert an existing archive.
• thin-to-full-archive.test now tests the contents to ensure the thin archives member have been added and not the archive itself. Added a test case for an existing archive and a case attempting to convert an existing archive.

Fixed the description after abrachet's comment and updated the help output to mirror the change.

LGTM

To be clear this new output is an approximation for cases in which the canonical line number is 0 and so not too helpful to the user. I wanted to suggest this functionality as we have seen cases in which this new option would be useful to a user. Suggestions for a better name than --approximate-missing-line-numbers would also be appreciated.

@MaskRay Thanks for clarifying.

Hi @MaskRay, apologies for the wait in this review. The change LGTM. Regarding the implicit conversion from thin to full archives I think there is a problem there, I've created https://reviews.llvm.org/D118693 to discuss it further.

I believe Gnu-ar commands without T on thin archives do not auto convert to "full" archives:

Hi @MaskRay, apologies for the wait in this review. I think James brought up a good point regarding gnu ar still using T. I realise that when moving towards consistency someone has to go first, but it maybe worth us waiting until they make the change so we maintain compatibility. Do we know if they have plans to move over to --thin?

This issue was resolved.

Due to a private patch we were seeing this warning cause a test failure, hence this diff.

LGTM but I will hold of accepting this revision until James has seen the responses.

I understand your hesitation @jhenderson regarding the loss of the short option, maybe a replacement short option could be found?

Hi @dmgreen, I think this change is the cause of CodeGen/ARM/fpclamptosat.ll failing on the clang-x64-windows-msvc build bot.

Fixed James' Nit and reduced the multiple identical calls to llvm-mc

I have also removed the successful test case from dwarf_invalid.yaml.

Fixed comment and added input to llvm-objdump call as suggested by James.

As suggested by dblaikie, now the call with summarize-types dumps -debug-types and not the other sections. To keep the useful check of the type_signature value it has been changed to the LONG prefix check.

Using -implicit-check-not=DW_TAG fails due to finding the string output in other sections, I think the explicit check might be required.

Updated after James' comments and fixed the format issue.

Good catch dblaikie, I missed that test and it covers almost all I was testing here. This updated revision adds a check that I think was missing from the existing test.

I was looking to do this in YAML but I don't think there is support for debug_types?

As suggested by Maskray I have renamed x86-64-reloc-32-fpic.s and added a R_X86_64_32 equivalent of the test. To be consistent x86-64-reloc-pc32-fpic.s was also renamed and the new test case added.
Note that git doesn't appear to detect the renames, it may be best for me to commit the code change and rename separately?

Just to confirm, this does not change the help output.

No problem MaskRay, thanks for the review.

Updated after Maskray's comment.

Thanks James, updated with your comments.

Updated after Maskray's comment.

Looking at the GNU docs its` --prefix=prefix`.

Fixed non windows test.

Looking at the other options in llvm-objdump, --option=<value> seems to be the consistent format. For this reason I changed the command line option rather than the user guide, if people disagree I'm happy to change the command guide instead.

This was committed in 05b1c7aebfff8cc08a620d01e739f343ed01db6d but I forgot to add the Differential Revision URL. As stated in the Code Reviews with Phabricator User Guide I will close this revision. Apologies for the noise.

Thanks for the revert @kda, I've reapplied with a fix.