Page MenuHomePhabricator

Ensure that the MRI CREATE/CREATETHIN commands overwrite the output file appropriately
ClosedPublic

Authored by bd1976llvm on Wed, May 11, 7:05 PM.

Details

Summary

The CREATE/CREATETHIN commands should overwrite the output file appropriately: https://sourceware.org/binutils/docs/binutils/ar-scripts.html.

This fixes a regression for MRI scripts introduced in: https://reviews.llvm.org/D123142 which put logic into performWriteOperation. performWriteOperation is called for all MRI commands that write an archive out (one's with a SAVE command). performWriteOperation is unaware of MRI semantics and loads an existing archive if present. If an existing archive is loaded, llvm-ar checks the properties of the existing archive for decisions about the output archive (for example making the output archive thin if the existing one was). https://reviews.llvm.org/D123142 adds the following logic...

if (OldArchive) {
  if (Thin && !OldArchive->isThin())
    fail("cannot convert a regular archive to a thin one");

  if (OldArchive->isThin())
    Thin = true;
}

... which errors for a script with CREATETHIN in effect if there is an existing regular archive, and causes CREATE to output a thin archive if there is an existing thin archive.

Diff Detail

Event Timeline

bd1976llvm created this revision.Wed, May 11, 7:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, May 11, 7:05 PM
Herald added a subscriber: rupprecht. · View Herald Transcript
bd1976llvm requested review of this revision.Wed, May 11, 7:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, May 11, 7:05 PM
jhenderson added inline comments.Thu, May 12, 12:21 AM
llvm/test/tools/llvm-ar/mri-create-overwrite.test
4

Let's cd into %t, then we don't need all these relative paths involving %t.

5

I recommend you move this line down to the first test case, as it's part of that specific test's setup code, and not a generic thing used by all the test cases.

10

If you cd into %t, you can use split-file instead of echo, since you don't need to dynamically generate the script contents. You can also avoid the need for touch to create the initial files, if you want, though that's less of an issue.

16

Check patterns are usually prefixed with a comment marker too, and are often then separated with a blank line to help them stand out. Aplpies throughout.

27

No need for a separate check pattern here. You can reuse the existing one.

Looking through all the test cases, I think you could get away with sharing the following block for all non-error cases:

# ARCH: !<arch>
# THIN:  !<thin>
# ONE:  1.txt
# TWO:  2.txt

and then use the appropriate check-prefixes incantation for the specific test case. Also, could you make the 1.txt and 2.txt lines -NEXT patterns?

29

If you were to move this test case before the CREATETHIN case above, you would already have a regular archive so wouldn't need to delete it and recreate it. Similar comments probably apply throughout the test file.

Additionally, here and throughout, the canonical term is "regular archive" not "full archive". Please remove all references to "full" from comments and check-directives.

37

Be consistent with double versus single dashes. Also, may not be relevant, but I'd prefer dashes to underscores in prefix names, as they're a) easier to type, and b) consistent with most other tests.

Applies throughout.

80

It seems like it would just be easier to show that archive.a still has it's old contents than make it read-only?

85–89

This seems to be somewhat tangential to what the test is doing: this is more about handling errors on SAVE, one of which might be a read-only file, but presumably there are other possible failure cases, e.g. disk space or bad paths that have nothing to do with the overwriting behaviour, but all go through the same code path on SAVE?

101

Nit: looks like you've got a double blank line at EOF?

llvm/tools/llvm-ar/llvm-ar.cpp
1141–1142

I'd add comment labels for the two nullptr args, to make it clear what they are for (fake examples suggested in the edit, to show formatting - names should match function parameter names).

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

bd1976llvm added inline comments.Thu, May 12, 7:29 AM
llvm/test/tools/llvm-ar/mri-create-overwrite.test
4

I'm not sure that this is the best approach because then you can't just copy/paste run lines from the lit output. You basically have to "cd" too. I'd rather leave this unless you feel strongly.

5

Thanks. Done.

10

Unless you feel strongly I would rather stick with echo. It's nice to keep all the pieces for a test-case together in the file.

16

Done. Thanks!

27

Thanks this makes the test more readable.

Also, could you make the 1.txt and 2.txt lines -NEXT patterns?

Sadly not, e.g:

1: !<thin>
2: // 72 `
3: C:/u/br2/test/tools/llvm-ar/Output/mri-create-overwrite.test.tmp/1.txt/

27

No need for a separate check pattern here. You can reuse the existing one.

Looking through all the test cases, I think you could get away with sharing the following block for all non-error cases:

# ARCH: !<arch>
# THIN:  !<thin>
# ONE:  1.txt
# TWO:  2.txt

and then use the appropriate check-prefixes incantation for the specific test case. Also, could you make the 1.txt and 2.txt lines -NEXT patterns?

29

I have changed terminology from full -> regular. Thanks.

Unless you feel strongly, I think that it is nice to keep the teste-cases self-contained rather than relying on state from the last test case. It makes the test easier to read.

37

Thanks. Done.

80

Done. Thanks.

85–89

Agreed. I have remove the test-case. Thanks.

101

Gone now. Thanks.

Improved test and addressed review comments.

jhenderson added inline comments.Thu, May 12, 7:44 AM
llvm/test/tools/llvm-ar/mri-create-overwrite.test
4

That's already the case though: you have to copy and paste the rm/mkdir line (as well as any other setup code), or you're not doing the same thing as the test did.

7–10

CHECK lines are usually after test cases.

10

As far as I'm aware, there's nothing stopping you doing that, unless llvm-ar doesn't support comment markers or blank lines in MRI scripts? Assuming it does, I think you can do:

# RUN: split-file %s
...
## Test case 1
# RUN: test case setup
# RUN: llvm-ar -M 1.mri
...
#--- 1.mri
<mri contents>

## Test case 2
...
#--- 2.mri

and so on.

llvm/tools/llvm-ar/llvm-ar.cpp
1059–1060

Don't modify lines unrelated to your change.

1141–1142

I was very deliberate with the = after the parameter name (but typo'ed it in the second case): clang-format recognises that specific pattern and doesn't insert a space between the comment and the name, which I find helps with readability.

bd1976llvm updated this revision to Diff 428957.EditedThu, May 12, 8:43 AM
bd1976llvm marked 2 inline comments as done.

Now using split-file and cd. Removed an invalid test-case. Addressed review comments.

llvm/test/tools/llvm-ar/mri-create-overwrite.test
4

Now cd-ing.

10

That worked. Also, there is quite a lot of duplication in the scripts so removing that outweighs keeping the script next to the test-cases.

LGTM, apart from a typo and a couple of unnecessary steps in the test.

llvm/test/tools/llvm-ar/mri-create-overwrite.test
4

You don't need the mkdir: split-file creates it automatically. (Keep the rm though).

9

This is noise: you start off the test deleting everything, and don't recreate the file again by this point. Delete it.

llvm/tools/llvm-ar/llvm-ar.cpp
1142

This line is apparently cursed to have typos in it :-D

bd1976llvm marked 12 inline comments as done.

Address final comments.

bd1976llvm marked 5 inline comments as done.Fri, May 13, 9:43 AM
bd1976llvm added inline comments.
llvm/tools/llvm-ar/llvm-ar.cpp
1142

:)

This revision was not accepted when it landed; it landed in state Needs Review.Fri, May 13, 3:50 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

LGTM.

https://sourceware.org/binutils/docs/binutils/ar-scripts.html.

It may be useful to leave a space before the period as many applications may consider . part of the URI.

llvm/tools/llvm-ar/llvm-ar.cpp
1142

clang-format will delete the space between /*OldArchiveBuf=*/ nullptr but it may not be necessary to modify the line now.

Hi, this test is failing on AIX because the default archive format is big archive. So when this test checks for !<arch> it sees <bigaf> instead.
https://lab.llvm.org/buildbot/#/builders/214/builds/1314/steps/6/logs/FAIL__LLVM__mri-create-overwrite_test

nkhoun added a subscriber: nkhoun.Mon, May 16, 2:48 PM

@bd1976llvm do you have an ETA to address the AIX failure Jake mentions above?

@bd1976llvm do you have an ETA to address the AIX failure Jake mentions above?

I'll have a look now. I have no knowledge of AIX but I think that Jake has already outlined what needs doing.

bd1976llvm marked an inline comment as done.Mon, May 16, 4:52 PM

@MaskRay - thanks for the review comments, I will keep your advise w.r.t. urls in commit comments in mind.

@Jake-Egan - should be fixed for AIX now, can you verify please.

@bd1976llvm
Thanks, the test is passing now.