Make BinaryStreamWriter::padToAlignment write blocks vs bytes.

[stellaraccident]

Summary

While I think this is a performance improvement over the original, this actually fixes a correctness issue: For an appendable underlying stream, padToAlignment would fail if the additional padding would have caused the stream to grow since it was doing its own check on bounds. By deferring to the regular writeArray method this takes the same path as everything else, which does the correct bounds check in WritableBinaryStreamRef::checkOffsetForWrite (i.e. skips the extension check if BSF_Append is set). I had started to fix the existing bounds check in BinaryStreamWriter but deferred to this because it layered better and is more efficient/consistent.

It didn't look like this method was tested at all, so I added a unit test.

[stellaraccident]

Add unit test.

[thakis]

Seems ok.

llvm/lib/Support/BinaryStreamWriter.cpp

98

nit: If you do

constexpr uint64_t ZerosSize = 64;
constexpr char Zeros[ZerosSize] = {};

then I think you don't need the memset call (and it's all constant zeroed data).

[thakis]

llvm/lib/Support/BinaryStreamWriter.cpp

98

@hans reminded me elsewhere that this is only true if you also make the array static, unless you build with -fmerge-all-constants: https://godbolt.org/z/fMcKjvfjo

So static constexpr Zeros[ZerosSize] = {} :)

[stellaraccident]

llvm/lib/Support/BinaryStreamWriter.cpp

98

@hans reminded me elsewhere that this is only true if you also make the array static, unless you build with -fmerge-all-constants: https://godbolt.org/z/fMcKjvfjo

So static constexpr Zeros[ZerosSize] = {} :)

Thank you - TIL today. Made the change as suggested.

[stellaraccident]

Address comments.

llvm/lib/Support/BinaryStreamWriter.cpp

BinaryStreamWriter::split(uint64_t Off) const {
   First = First.keep_front(Off);
   BinaryStreamWriter W1{First};
   BinaryStreamWriter W2{Second};
   return std::make_pair(W1, W2);
 }
 
 Error BinaryStreamWriter::padToAlignment(uint32_t Align) {
   uint64_t NewOffset = alignTo(Offset, Align);
-  if (NewOffset > getLength())
-    return make_error<BinaryStreamError>(stream_error_code::stream_too_short);
+  const uint64_t ZerosSize = 64;
+  static constexpr char Zeros[ZerosSize] = {};
   while (Offset < NewOffset)
-    if (auto EC = writeInteger('\0'))
-      return EC;
+    if (auto E = writeArray(
+            ArrayRef<char>(Zeros, std::min(ZerosSize, NewOffset - Offset))))
+      return E;
   return Error::success();
 }

llvm/unittests/Support/BinaryStreamTest.cpp

for (auto &Stream : Streams) {
     BinaryStreamReader ReaderBacker(*Stream.Output);
     ASSERT_THAT_ERROR(ReaderBacker.readArray(Ints2, SourceInts.size()),
                       Succeeded());
 
     EXPECT_EQ(makeArrayRef(SourceInts), Ints2);
   }
 }
 
-TEST_F(BinaryStreamTest, StringWriterStrings) {
+TEST_F(BinaryStreamTest, StreamWriterStrings) {
   StringRef Strings[] = {"First", "Second", "Third", "Fourth"};
 
   size_t Length = 0;
   for (auto S : Strings)
     Length += S.size() + 1;
   initializeOutput(Length, 1);
   initializeInputFromOutput(1);
 
   for (auto &Stream : Streams) {
     BinaryStreamWriter Writer(*Stream.Output);
     for (auto S : Strings)
       ASSERT_THAT_ERROR(Writer.writeCString(S), Succeeded());
     std::vector<StringRef> InStrings;
     BinaryStreamReader Reader(*Stream.Input);
     while (!Reader.empty()) {
       StringRef S;
       ASSERT_THAT_ERROR(Reader.readCString(S), Succeeded());
       InStrings.push_back(S);
     }
     EXPECT_EQ(makeArrayRef(Strings), makeArrayRef(InStrings));
   }
 }
 
+TEST_F(BinaryStreamTest, StreamWriterPadToAlignment) {
+  // This test may seem excessive but it is checking for past bugs and corner
+  // cases by making sure that the stream is allowed to grow and that
+  // both multiple pad chunks and single chunk extensions work.
+  AppendingBinaryByteStream Stream(support::little);
+  BinaryStreamWriter Writer(Stream);
+
+  // Offset 0: '0'
+  EXPECT_THAT_ERROR(Writer.writeInteger('0'), Succeeded());
+  // Offset 1..110: 0
+  EXPECT_THAT_ERROR(Writer.padToAlignment(111), Succeeded());
+  // Offset 111: '*'
+  EXPECT_THAT_ERROR(Writer.writeInteger('*'), Succeeded());
+  // Offset 112..120: 0
+  EXPECT_THAT_ERROR(Writer.padToAlignment(11), Succeeded());
+
+  BinaryStreamReader Reader(Stream);
+  char c;
+  // Offset 0
+  EXPECT_THAT_ERROR(Reader.readInteger<char>(c), Succeeded());
+  EXPECT_EQ('0', c);
+  // Offset 1..110
+  for (int i = 0; i < 110; ++i) {
+    char c;
+    EXPECT_THAT_ERROR(Reader.readInteger<char>(c), Succeeded());
+    EXPECT_EQ('\0', c);
+  }
+  // Offset 111
+  EXPECT_THAT_ERROR(Reader.readInteger<char>(c), Succeeded());
+  EXPECT_EQ('*', c);
+  // Offset 112..120
+  for (int i = 0; i < 9; ++i) {
+    char c;
+    EXPECT_THAT_ERROR(Reader.readInteger<char>(c), Succeeded());
+    EXPECT_EQ('\0', c);
+  }
+
+  // EOF.
+  EXPECT_THAT_ERROR(Reader.readInteger<char>(c), Failed());
+}
+
 TEST_F(BinaryStreamTest, StreamWriterAppend) {
   StringRef Strings[] = {"First", "Second", "Third", "Fourth"};
   AppendingBinaryByteStream Stream(support::little);
   BinaryStreamWriter Writer(Stream);
 
   for (auto &Str : Strings) {
     EXPECT_THAT_ERROR(Writer.writeCString(Str), Succeeded());
   }