[llvm-ar] Create thin archives with MRI scripts

[gbreynoo]

Summary

This patch implements the "CREATE_THIN" MRI script command, allowing thin archives to be created via MRI scripts. It is not found in gnu-ar but as it has been useful to some of our customers, I thought it worth putting up for review.

[rupprecht]

I guess there are no rules for MRI script compatibility -- but I wonder how much work we want to invest in improving MRI support beyond supporting people migrating from GNU ar (even there, MRI support is only added to support people migrating from a different librarian program...). This seems like a minor change to allow, but I just wanted to say that before you go crazy adding things :)

tools/llvm-ar/llvm-ar.cpp

999 ↗

(On Diff #203180)

tiny nit: remove extra whitespace before the colon

1000 ↗

(On Diff #203180)

Add LLVM_FALLTHROUGH; after this to indicate intentional fallthrough

1000 ↗

(On Diff #203180)

If you have an MRI script like this:

createthin foo.a
...
save
create bar.a
...
save

Then bar.a ought be a normal archive (because it was not created with createthin), but since Thin is a global variable here, it will be a thin archive. Is that correct? (Can you add a test for that?) I think you need to avoid having a fall through, i.e. the case MRICommand::Create should explicitly set Thin = false, and likewise case MRICommand::CreateThin should explicitly set it to true.

Alternatively, another solution is to provide thin and regular (or notthin, 'fat', or whatever opposite name you want) that toggles the global state.

[rupprecht]

(LGTM other than the comment about Thin being global tho)

[gbreynoo]

Changes after rupprecht's comments.

[ruiu]

Looks like this patch doesn't still fix the issue that rupprecht pointed out. If an MRI script looks like this

createthin foo.a
...
save
create bar.a
...
save

doesn't thes second "create" create a thin archive?

[gbreynoo]

tools/llvm-ar/llvm-ar.cpp

1000 ↗

(On Diff #203180)

Unfortunately a limitation of the current implementation is that editing multiple archives in one MRI script is not supported. In the event CREATE (or now CREATETHIN) are called more than once the whole operation fails. I'm happy to separate the two cases but at the moment there will not be a behaviour change.

[ruiu]

I'm not sure if I'm a right person to make a decision whether adding a new MRI command is OK or not, but looks like this new command makes sense, and no one seems to be opposing, so LGTM.

tools/llvm-ar/llvm-ar.cpp

1000 ↗

(On Diff #203180)

Ah, thanks for the explanation. If that's the case this looks fine.

llvm/trunk/test/tools/llvm-ar/mri-thin-archive.test

+RUN: rm -rf %t && mkdir -p %t/addlib/
+
+RUN: yaml2obj %S/Inputs/elf.yaml -o %t/elf.o
+RUN: cp %t/elf.o %t/addlib/elf.o
+RUN: cp %t/elf.o %t/delete.o
+
+RUN: cd %t && llvm-ar rTc addlib/addlib.ar addlib/elf.o
+
+RUN: echo "createthin %t/archive.ar" > %t/mri.script
+RUN: echo "addmod elf.o" >> %t/mri.script
+RUN: echo "addlib addlib/addlib.ar" >> %t/mri.script
+RUN: echo "addmod delete.o" >> %t/mri.script
+RUN: echo "delete delete.o" >> %t/mri.script
+RUN: echo "save" >> %t/mri.script
+RUN: echo "end" >> %t/mri.script
+
+RUN: cd %t && llvm-ar -M < mri.script
+RUN: FileCheck -input-file=%t/archive.ar %s
+
+CHECK: !<thin>
+CHECK: elf.o
+CHECK-NEXT: addlib/elf.o/
+CHECK-NOT: delete.o

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

if (!shouldCreateArchive(Operation)) {
     }
   }
 
   performOperation(Operation, nullptr, nullptr, NewMembers);
   return 0;
 }
 
 static void runMRIScript() {
-  enum class MRICommand { AddLib, AddMod, Create, Delete, Save, End, Invalid };
+  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();
   bool Saved = false;
   std::vector<NewArchiveMember> NewMembers;
 
   for (line_iterator I(Ref, /*SkipBlanks*/ false), E; I != E; ++I) {
     StringRef Line = *I;
     Line = Line.split(';').first;
     Line = Line.split('*').first;
     Line = Line.trim();
     if (Line.empty())
       continue;
     StringRef CommandStr, Rest;
     std::tie(CommandStr, Rest) = Line.split(' ');
     Rest = Rest.trim();
     if (!Rest.empty() && Rest.front() == '"' && Rest.back() == '"')
       Rest = Rest.drop_front().drop_back();
     auto Command = StringSwitch<MRICommand>(CommandStr.lower())
                        .Case("addlib", MRICommand::AddLib)
                        .Case("addmod", MRICommand::AddMod)
                        .Case("create", MRICommand::Create)
+                       .Case("createthin", MRICommand::CreateThin)
                        .Case("delete", MRICommand::Delete)
                        .Case("save", MRICommand::Save)
                        .Case("end", MRICommand::End)
                        .Default(MRICommand::Invalid);
 
     switch (Command) {
     case MRICommand::AddLib: {
       object::Archive &Lib = readLibrary(Rest);
       {
         Error Err = Error::success();
         for (auto &Member : Lib.children(Err))
           addChildMember(NewMembers, Member, /*FlattenArchive=*/Thin);
         failIfError(std::move(Err));
       }
       break;
     }
     case MRICommand::AddMod:
       addMember(NewMembers, Rest);
       break;
+    case MRICommand::CreateThin:
+      Thin = true;
+      LLVM_FALLTHROUGH;
     case MRICommand::Create:
       Create = true;
       if (!ArchiveName.empty())
         fail("Editing multiple archives not supported");
       if (Saved)
         fail("File already saved");
       ArchiveName = Rest;
       break;