User Details
- User Since
- Jan 19 2017, 6:04 AM (278 w, 4 d)
Thu, May 12
Thanks for fixing this Ben, once James' suggestions are fixed it LGTM.
Thu, Apr 28
@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.
Wed, Apr 27
Updated with urnathan's suggestion.
Tue, Apr 26
This looks like a good change to me, just needs that ranlib test fixing.
Move testing to area suggested by @urnathan.
Apr 21 2022
Apr 20 2022
Fixed failing test and @jhenderson's other points.
Apr 19 2022
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.
Apr 14 2022
Good idea @MaskRay, I've split that into https://reviews.llvm.org/D123778 and will update this one when D123778 is complete.
Apr 13 2022
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.
Apr 5 2022
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.
Apr 4 2022
Apr 1 2022
Fixed the description after abrachet's comment and updated the help output to mirror the change.
Mar 17 2022
LGTM
Feb 28 2022
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.
Feb 3 2022
Feb 2 2022
@MaskRay Thanks for clarifying.
Feb 1 2022
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?
Jan 17 2022
This issue was resolved.
Jan 13 2022
Due to a private patch we were seeing this warning cause a test failure, hence this diff.
Jan 11 2022
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?
Jan 5 2022
Jan 4 2022
Dec 7 2021
Dec 6 2021
Dec 1 2021
Hi @dmgreen, I think this change is the cause of CodeGen/ARM/fpclamptosat.ll failing on the clang-x64-windows-msvc build bot.
Nov 4 2021
Nov 3 2021
Fixed James' Nit and reduced the multiple identical calls to llvm-mc
Nov 2 2021
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.
Nov 1 2021
Oct 29 2021
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.
Oct 25 2021
Using -implicit-check-not=DW_TAG fails due to finding the string output in other sections, I think the explicit check might be required.
Oct 22 2021
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.
Oct 20 2021
I was looking to do this in YAML but I don't think there is support for debug_types?
Oct 18 2021
Oct 15 2021
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?
Oct 14 2021
Just to confirm, this does not change the help output.
Oct 13 2021
Oct 7 2021
No problem MaskRay, thanks for the review.
Oct 6 2021
Sep 30 2021
Updated after Maskray's comment.
Thanks James, updated with your comments.
Sep 28 2021
Updated after Maskray's comment.
Sep 27 2021
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.
Sep 24 2021
Sep 23 2021
Sep 3 2021
Thanks for the revert @kda, I've reapplied with a fix.