[Support/TarWriter] - Don't allow TarWriter to add the same file more than once.

[grimar]

Summary

This is for PR35460.

Currently when LLD adds files to TarWriter it may pass the same file multiple times.
For example it happens for clang reproduce file which specifies archive (.a) files more
than once in command line. Though that can be catched on LLD side, I think it is reasonable
to restrict TarWriter to add the same file more than once, I believe current behavior does not
make much sence.

[ruiu]

include/llvm/Support/TarWriter.h

16 ↗

(On Diff #124755)

Please use llvm::StringSet.

24 ↗

(On Diff #124755)

Just keep it return void as you are not going to use its return value.

lib/Support/TarWriter.cpp

176 ↗

(On Diff #124755)

Exists

177–178 ↗

(On Diff #124755)

Or, just if (Seen.insert(Fullpath).second) return;

[grimar]

• Addressed review comments.

[grimar]

include/llvm/Support/TarWriter.h

16 ↗

(On Diff #124755)

It does not work:

>D:\Work\llvm\include\llvm/ADT/StringMap.h(209): error C2039: 'Deallocate': is not a member of 'std::basic_string<char,std::char_traits<char>,std::allocator<char>>'
1>  C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\xstring(2633): note: see declaration of 'std::basic_string<char,std::char_traits<char>,std::allocator<char>>'
1>  D:\Work\llvm\include\llvm/ADT/StringMap.h(301): note: see reference to function template instantiation 'void llvm::StringMapEntry<ValueTy>::Destroy<AllocatorTy>(AllocatorTy &)' being compiled
1>          with
1>          [
1>              ValueTy=char,
1>              AllocatorTy=std::string
1>          ]

I think llvm containers does not know how to handle std::string. So I had to use std::set. It seems common in LLVM to use it for std::string.

lib/Support/TarWriter.cpp

177–178 ↗

(On Diff #124755)

That looks much better, thanks !

[grimar]

• Use llvm::StringSet instead of std::set<std::string>.

[ruiu]

LGTM with a test.

[grimar]

• Added testcase.

[ruiu]

unittests/Support/TarWriterTest.cpp

124–143 ↗

(On Diff #125095)

You should split this into three different test functions.

[grimar]

Addressed review comments:

• Split test into three.
• Use llvm::sys::fs::file_size API for finding output tar file size.

[ruiu]

LGTM with this change.

unittests/Support/TarWriterTest.cpp

142 ↗

(On Diff #125298)

These test names made me think for a while why you needed to check for file size (e.g. did we have a logic to pick the largest file or something?)

It turned out that you are using file size just to verify that output is correct.

Please rename these tests SingleFile, NoDuplicate and Duplicate.

llvm/trunk/include/llvm/Support/TarWriter.h

 //===-- llvm/Support/TarWriter.h - Tar archive file creator -----*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_SUPPORT_TAR_WRITER_H
 #define LLVM_SUPPORT_TAR_WRITER_H
 
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/raw_ostream.h"
 
 namespace llvm {
 class TarWriter {
 public:
   static Expected<std::unique_ptr<TarWriter>> create(StringRef OutputPath,
                                                      StringRef BaseDir);
 
   void append(StringRef Path, StringRef Data);
 
 private:
   TarWriter(int FD, StringRef BaseDir);
   raw_fd_ostream OS;
   std::string BaseDir;
+  StringSet<> Files;
 };
 }
 
 #endif

llvm/trunk/lib/Support/TarWriter.cpp

 TarWriter::TarWriter(int FD, StringRef BaseDir)
     : OS(FD, /*shouldClose=*/true, /*unbuffered=*/false), BaseDir(BaseDir) {}
 
 // Append a given file to an archive.
 void TarWriter::append(StringRef Path, StringRef Data) {
   // Write Path and Data.
   std::string Fullpath = BaseDir + "/" + sys::path::convert_to_slash(Path);
 
+  // We do not want to include the same file more than once.
+  if (!Files.insert(Fullpath).second)
+    return;
+
   StringRef Prefix;
   StringRef Name;
   if (splitUstar(Fullpath, Prefix, Name)) {
     writeUstarHeader(OS, Prefix, Name, Data.size());
   } else {
     writePaxHeader(OS, Fullpath);
     writeUstarHeader(OS, "", "", Data.size());
   }

llvm/trunk/unittests/Support/TarWriterTest.cpp

TEST_F(TarWriterTest, Pax) {
 
   auto *Hdr = reinterpret_cast<const UstarHeader *>(Buf.data());
   EXPECT_EQ("", StringRef(Hdr->Prefix));
   EXPECT_EQ("", StringRef(Hdr->Name));
 
   StringRef Pax = StringRef((char *)(Buf.data() + 512), 512);
   EXPECT_TRUE(Pax.startswith("211 path=/" + std::string(200, 'x')));
 }
+
+TEST_F(TarWriterTest, SingleFile) {
+  SmallString<128> Path;
+  std::error_code EC =
+      sys::fs::createTemporaryFile("TarWriterTest", "tar", Path);
+  EXPECT_FALSE((bool)EC);
+
+  Expected<std::unique_ptr<TarWriter>> TarOrErr = TarWriter::create(Path, "");
+  EXPECT_TRUE((bool)TarOrErr);
+  std::unique_ptr<TarWriter> Tar = std::move(*TarOrErr);
+  Tar->append("FooPath", "foo");
+  Tar.reset();
+
+  uint64_t TarSize;
+  EC = sys::fs::file_size(Path, TarSize);
+  EXPECT_FALSE((bool)EC);
+  EXPECT_EQ(TarSize, 2048);
+}
+
+TEST_F(TarWriterTest, NoDuplicate) {
+  SmallString<128> Path;
+  std::error_code EC =
+      sys::fs::createTemporaryFile("TarWriterTest", "tar", Path);
+  EXPECT_FALSE((bool)EC);
+
+  Expected<std::unique_ptr<TarWriter>> TarOrErr = TarWriter::create(Path, "");
+  EXPECT_TRUE((bool)TarOrErr);
+  std::unique_ptr<TarWriter> Tar = std::move(*TarOrErr);
+  Tar->append("FooPath", "foo");
+  Tar->append("BarPath", "bar");
+  Tar.reset();
+
+  uint64_t TarSize;
+  EC = sys::fs::file_size(Path, TarSize);
+  EXPECT_FALSE((bool)EC);
+  EXPECT_EQ(TarSize, 3072);
+}
+
+TEST_F(TarWriterTest, Duplicate) {
+  SmallString<128> Path;
+  std::error_code EC =
+      sys::fs::createTemporaryFile("TarWriterTest", "tar", Path);
+  EXPECT_FALSE((bool)EC);
+
+  Expected<std::unique_ptr<TarWriter>> TarOrErr = TarWriter::create(Path, "");
+  EXPECT_TRUE((bool)TarOrErr);
+  std::unique_ptr<TarWriter> Tar = std::move(*TarOrErr);
+  Tar->append("FooPath", "foo");
+  Tar->append("FooPath", "bar");
+  Tar.reset();
+
+  uint64_t TarSize;
+  EC = sys::fs::file_size(Path, TarSize);
+  EXPECT_FALSE((bool)EC);
+  EXPECT_EQ(TarSize, 2048);
 }
+} // namespace