[PATCH] D115331: [llvm] Add null-termination capability to SmallVectorMemoryBuffer

2021-12-09 Thread Jan Svoboda via Phabricator via cfe-commits 
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGd0262c2394f4: [llvm] Add null-termination capability to 
SmallVectorMemoryBuffer (authored by jansvoboda11).

Changed prior to commit:
  https://reviews.llvm.org/D115331?vs=392716=393080#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115331/new/

https://reviews.llvm.org/D115331

Files:
  clang/unittests/AST/ASTImporterTest.cpp
  clang/unittests/Frontend/TextDiagnosticTest.cpp
  llvm/include/llvm/Support/SmallVectorMemoryBuffer.h
  llvm/lib/ExecutionEngine/MCJIT/MCJIT.cpp
  llvm/lib/ExecutionEngine/Orc/CompileUtils.cpp
  llvm/lib/LTO/ThinLTOCodeGenerator.cpp
  llvm/lib/Object/ArchiveWriter.cpp
  llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
  llvm/tools/llvm-objcopy/llvm-objcopy.cpp
  llvm/unittests/Support/MemoryBufferTest.cpp

Index: llvm/unittests/Support/MemoryBufferTest.cpp
===
--- llvm/unittests/Support/MemoryBufferTest.cpp
+++ llvm/unittests/Support/MemoryBufferTest.cpp
@@ -11,6 +11,7 @@
 //===--===//
 
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/SmallVectorMemoryBuffer.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FileUtilities.h"
@@ -408,4 +409,27 @@
   EXPECT_EQ(MB->getBufferSize(), std::size_t(FileWrites * 8));
   EXPECT_TRUE(MB->getBuffer().startswith("01234567"));
 }
+
+// Test that SmallVector without a null terminator gets one.
+TEST(SmallVectorMemoryBufferTest, WithoutNullTerminatorRequiresNullTerminator) {
+  SmallString<0> Data("some data");
+
+  SmallVectorMemoryBuffer MB(std::move(Data),
+ /*RequiresNullTerminator=*/true);
+  EXPECT_EQ(MB.getBufferSize(), 9u);
+  EXPECT_EQ(MB.getBufferEnd()[0], '\0');
+}
+
+// Test that SmallVector with a null terminator keeps it.
+TEST(SmallVectorMemoryBufferTest, WithNullTerminatorRequiresNullTerminator) {
+  SmallString<0> Data("some data");
+  Data.push_back('\0');
+  Data.pop_back();
+
+  SmallVectorMemoryBuffer MB(std::move(Data),
+ /*RequiresNullTerminator=*/true);
+  EXPECT_EQ(MB.getBufferSize(), 9u);
+  EXPECT_EQ(MB.getBufferEnd()[0], '\0');
 }
+
+} // namespace
Index: llvm/tools/llvm-objcopy/llvm-objcopy.cpp
===
--- llvm/tools/llvm-objcopy/llvm-objcopy.cpp
+++ llvm/tools/llvm-objcopy/llvm-objcopy.cpp
@@ -236,7 +236,8 @@
   return createFileError(Ar.getFileName(), Member.takeError());
 
 Member->Buf = std::make_unique(
-std::move(Buffer), ChildNameOrErr.get());
+std::move(Buffer), ChildNameOrErr.get(),
+/*RequiresNullTerminator=*/false);
 Member->MemberName = Member->Buf->getBufferIdentifier();
 NewArchiveMembers.push_back(std::move(*Member));
   }
Index: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
===
--- llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
+++ llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
@@ -476,9 +476,8 @@
  **ObjOrErr, MemStream))
   return E;
 
-std::unique_ptr MB =
-std::make_unique(std::move(Buffer),
-  ArchFlagName);
+auto MB = std::make_unique(
+std::move(Buffer), ArchFlagName, /*RequiresNullTerminator=*/false);
 Expected> BinaryOrErr = object::createBinary(*MB);
 if (!BinaryOrErr)
   return BinaryOrErr.takeError();
Index: llvm/lib/Object/ArchiveWriter.cpp
===
--- llvm/lib/Object/ArchiveWriter.cpp
+++ llvm/lib/Object/ArchiveWriter.cpp
@@ -696,7 +696,7 @@
 return std::move(E);
 
   return std::make_unique(
-  std::move(ArchiveBufferVector));
+  std::move(ArchiveBufferVector), /*RequiresNullTerminator=*/false);
 }
 
 } // namespace llvm
Index: llvm/lib/LTO/ThinLTOCodeGenerator.cpp
===
--- llvm/lib/LTO/ThinLTOCodeGenerator.cpp
+++ llvm/lib/LTO/ThinLTOCodeGenerator.cpp
@@ -378,7 +378,8 @@
 // Run codegen now. resulting binary is in OutputBuffer.
 PM.run(TheModule);
   }
-  return std::make_unique(std::move(OutputBuffer));
+  return std::make_unique(
+  std::move(OutputBuffer), /*RequiresNullTerminator=*/false);
 }
 
 /// Manage caching for a single Module.
@@ -541,7 +542,8 @@
   auto Index = buildModuleSummaryIndex(TheModule, nullptr, );
   WriteBitcodeToFile(TheModule, OS, true, );
 }
-return std::make_unique(std::move(OutputBuffer));
+return std::make_unique(
+std::move(OutputBuffer), /*RequiresNullTerminator=*/false);
   }
 
   return codegenModule(TheModule, TM);
Index:

[PATCH] D115331: [llvm] Add null-termination capability to SmallVectorMemoryBuffer

2021-12-08 Thread Duncan P. N. Exon Smith via Phabricator via cfe-commits 
dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM, except I'd prefer the `#include`s be changed separately since that 
cleanup seems unrelated to this change.




Comment at: lldb/unittests/Expression/CppModuleConfigurationTest.cpp:14
 #include "lldb/Host/HostInfo.h"
-#include "llvm/Support/SmallVectorMemoryBuffer.h"
 

Please clean up the includes in a prep commit (no need for review IMO) rather 
than a drive-by.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115331/new/

https://reviews.llvm.org/D115331

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D115331: [llvm] Add null-termination capability to SmallVectorMemoryBuffer

2021-12-08 Thread Jan Svoboda via Phabricator via cfe-commits 
jansvoboda11 created this revision.
jansvoboda11 added reviewers: Bigcheese, dexonsmith.
Herald added subscribers: ormris, steven_wu, hiraditya.
Herald added a reviewer: alexander-shaposhnikov.
Herald added a reviewer: shafik.
Herald added a reviewer: rupprecht.
Herald added a reviewer: jhenderson.
jansvoboda11 requested review of this revision.
Herald added subscribers: llvm-commits, lldb-commits, cfe-commits, MaskRay.
Herald added projects: clang, LLDB, LLVM.

Most of `MemoryBuffer` interfaces expose a `RequiresNullTerminator` parameter 
that's being used to:

- determine how to open a file (`mmap` vs `open`),
- assert newly initialized buffer indeed has an implicit null terminator.

This patch adds the paramater to the `SmallVectorMemoryBuffer` constructors, 
meaning:

- null terminator can now be added to `SmallVector`s that didn't have one 
before,
- `SmallVectors` that had a null terminator before keep it even after the move.

In line with existing code, the new parameter is defaulted to `true`. This 
patch makes sure all calls to the `SmallVectorMemoryBuffer` constructor set it 
to `false` to preserve the current semantics.

As a drive-by fix, this patch removes unused `#include`s of 
`SmallVectorMemoryBuffer.h` and ensures all callers use the `auto Var = 
std::make_unique(...);` pattern.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D115331

Files:
  clang/unittests/AST/ASTImporterTest.cpp
  clang/unittests/Frontend/TextDiagnosticTest.cpp
  lldb/unittests/Expression/CppModuleConfigurationTest.cpp
  llvm/include/llvm/Support/SmallVectorMemoryBuffer.h
  llvm/lib/ExecutionEngine/MCJIT/MCJIT.cpp
  llvm/lib/ExecutionEngine/Orc/CompileUtils.cpp
  llvm/lib/LTO/LTOBackend.cpp
  llvm/lib/LTO/ThinLTOCodeGenerator.cpp
  llvm/lib/Object/ArchiveWriter.cpp
  llvm/lib/Object/MachOUniversalWriter.cpp
  llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
  llvm/tools/llvm-objcopy/llvm-objcopy.cpp
  llvm/unittests/Support/MemoryBufferTest.cpp

Index: llvm/unittests/Support/MemoryBufferTest.cpp
===
--- llvm/unittests/Support/MemoryBufferTest.cpp
+++ llvm/unittests/Support/MemoryBufferTest.cpp
@@ -11,6 +11,7 @@
 //===--===//
 
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/SmallVectorMemoryBuffer.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FileUtilities.h"
@@ -408,4 +409,27 @@
   EXPECT_EQ(MB->getBufferSize(), std::size_t(FileWrites * 8));
   EXPECT_TRUE(MB->getBuffer().startswith("01234567"));
 }
+
+// Test that SmallVector without a null terminator gets one.
+TEST(SmallVectorMemoryBufferTest, WithoutNullTerminatorRequiresNullTerminator) {
+  SmallString<0> Data("some data");
+
+  SmallVectorMemoryBuffer MB(std::move(Data),
+ /*RequiresNullTerminator=*/true);
+  EXPECT_EQ(MB.getBufferSize(), 9u);
+  EXPECT_EQ(MB.getBufferEnd()[0], '\0');
+}
+
+// Test that SmallVector with a null terminator keeps it.
+TEST(SmallVectorMemoryBufferTest, WithNullTerminatorRequiresNullTerminator) {
+  SmallString<0> Data("some data");
+  Data.push_back('\0');
+  Data.pop_back();
+
+  SmallVectorMemoryBuffer MB(std::move(Data),
+ /*RequiresNullTerminator=*/true);
+  EXPECT_EQ(MB.getBufferSize(), 9u);
+  EXPECT_EQ(MB.getBufferEnd()[0], '\0');
 }
+
+} // namespace
Index: llvm/tools/llvm-objcopy/llvm-objcopy.cpp
===
--- llvm/tools/llvm-objcopy/llvm-objcopy.cpp
+++ llvm/tools/llvm-objcopy/llvm-objcopy.cpp
@@ -236,7 +236,8 @@
   return createFileError(Ar.getFileName(), Member.takeError());
 
 Member->Buf = std::make_unique(
-std::move(Buffer), ChildNameOrErr.get());
+std::move(Buffer), ChildNameOrErr.get(),
+/*RequiresNullTerminator=*/false);
 Member->MemberName = Member->Buf->getBufferIdentifier();
 NewArchiveMembers.push_back(std::move(*Member));
   }
Index: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
===
--- llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
+++ llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
@@ -476,9 +476,8 @@
  **ObjOrErr, MemStream))
   return E;
 
-std::unique_ptr MB =
-std::make_unique(std::move(Buffer),
-  ArchFlagName);
+auto MB = std::make_unique(
+std::move(Buffer), ArchFlagName, /*RequiresNullTerminator=*/false);
 Expected> BinaryOrErr = object::createBinary(*MB);
 if (!BinaryOrErr)
   return BinaryOrErr.takeError();
Index: llvm/lib/Object/MachOUniversalWriter.cpp
===
--- llvm/lib/Object/MachOUniversalWriter.cpp
+++ llvm/lib/Object/MachOUniversalWriter.cpp
@@ -19,7 +19,6 @@
 #include