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

[bd1976llvm]

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.

[jhenderson]

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

1137–1138

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).

[gbreynoo]

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

[bd1976llvm]

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.

[bd1976llvm]

Improved test and addressed review comments.

[jhenderson]

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

1055–1056

Don't modify lines unrelated to your change.

1137–1138

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]

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.

[jhenderson]

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

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

3

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

8

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

1138

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

[bd1976llvm]

Address final comments.

[bd1976llvm]

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

1138

:)

[MaskRay]

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

1138

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

[Jake-Egan]

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]

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

[bd1976llvm]

In D125439#3517356, @nkhoun wrote:

@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]

@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.

[Jake-Egan]

@bd1976llvm
Thanks, the test is passing now.

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

+## Show that CREATE/CREATETHIN overwrite existing files appropriately.
+
+# RUN: rm -rf %t && mkdir -p %t
+# RUN: split-file %s %t
+# RUN: cd %t
+
+## Show that an existing file that is not an archive is overwritten by CREATE.
+# RUN: rm -f test.a
+# RUN: touch test.a
+# RUN: llvm-ar -M < create.mri
+# RUN: FileCheck --input-file=test.a %s --check-prefixes=ARCH,TWO
+
+## Show that an existing file that is not an archive is overwritten by CREATETHIN.
+# RUN: rm -f test.a
+# RUN: touch test.a
+# RUN: llvm-ar -M < createthin.mri
+# RUN: FileCheck --input-file=test.a %s --check-prefixes=THIN,TWO
+
+## Show that an existing regular archive is overwritten by CREATE.
+# RUN: rm -f test.a
+# RUN: llvm-ar cr test.a 1.txt
+# RUN: llvm-ar -M < create.mri
+# RUN: llvm-ar tv test.a
+# RUN: FileCheck --input-file=test.a %s --check-prefixes=ARCH,TWO --implicit-check-not=1.txt
+
+## Show that an existing regular archive is overwritten by CREATETHIN.
+# RUN: rm -f test.a
+# RUN: llvm-ar cr test.a 1.txt
+# RUN: llvm-ar -M < createthin.mri
+# RUN: FileCheck --input-file=test.a %s --check-prefixes=THIN,TWO --implicit-check-not=1.txt
+
+## Show that an existing thin archive is overwritten by CREATE.
+# RUN: rm -f test.a
+# RUN: llvm-ar crT test.a 1.txt
+# RUN: llvm-ar -M < create.mri
+# RUN: FileCheck --input-file=test.a %s --check-prefixes=ARCH,TWO --implicit-check-not=1.txt
+
+## Show that an existing thin archive is overwritten by CREATETHIN.
+# RUN: rm -f test.a
+# RUN: llvm-ar crT test.a 1.txt
+# RUN: llvm-ar -M < createthin.mri
+# RUN: FileCheck --input-file=test.a %s --check-prefixes=THIN,TWO --implicit-check-not=1.txt
+
+## Show that the output is not overwritten without a SAVE.
+# RUN: rm -f test.a
+# RUN: llvm-ar crT test.a 1.txt
+# RUN: llvm-ar -M < nosave.mri
+# RUN: FileCheck --input-file=test.a %s --check-prefixes=THIN,ONE --implicit-check-not=2.txt
+
+# ARCH: !<arch>
+# THIN: !<thin>
+# ONE:  1.txt
+# TWO:  2.txt
+
+#--- 1.txt
+
+#--- 2.txt
+
+#--- create.mri
+CREATE test.a
+ADDMOD 2.txt
+SAVE
+
+#--- createthin.mri
+CREATETHIN test.a
+ADDMOD 2.txt
+SAVE
+
+#--- nosave.mri
+CREATE test.a
+ADDMOD 2.txt

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

static int performOperation(ArchiveOperation Operation,
   } else {
     if (!Create) {
       // Produce a warning if we should and we're creating the archive
       WithColor::warning(errs(), ToolName)
           << "creating " << ArchiveName << "\n";
     }
   }
 
   performOperation(Operation, nullptr, nullptr, NewMembers);
   return 0;
 }
 
 static void runMRIScript() {
   enum class MRICommand { AddLib, AddMod, Create, CreateThin, Delete, Save, End, Invalid };
 
   ErrorOr<std::unique_ptr<MemoryBuffer>> Buf = MemoryBuffer::getSTDIN();
   failIfError(Buf.getError());
   const MemoryBuffer &Ref = *Buf.get();
case MRICommand::Invalid:
       fail("unknown command: " + CommandStr);
     }
   }
 
   ParsingMRIScript = false;
 
   // Nothing to do if not saved.
   if (Saved)
-    performOperation(ReplaceOrInsert, &NewMembers);
+    performOperation(ReplaceOrInsert, /*OldArchive=*/nullptr,
+                     /*OldArchiveBu=f*/ nullptr, &NewMembers);
   exit(0);
 }
 
 static bool handleGenericOption(StringRef arg) {
   if (arg == "--help" || arg == "-h") {
     printHelpMessage();
     return true;
   }