[llvm-rc] Exclude padding from sizes in versioninfo resources

[mstorsjo]

Summary

Normally when writing something that requires padding, we first measure the length of the written payload data, then write padding if necessary.

For a recursive structure like versioninfo, this means that the padding is excluded from the size of the inner element, but included in the size of the enclosing block.

Rc.exe excludes the final padding (but not the padding of earlier children) from all levels of the hierarchy.

To achieve this, don't pad after each block or value, but only before starting the next one. We still pad after completing the toplevel versioninfo resource, so this won't affect other resource types.

[zturner]

Nice fix. I can also confirm that the new output is byte-identical to rc.exe's output for the new test case, and that our old output was different.

test/tools/llvm-rc/Inputs/versioninfo-padding.rc

+1 VERSIONINFO
+FILETYPE 2
+FILEOS 4
+PRODUCTVERSION 4,0,0,0
+FILEVERSION 4,0,0,0
+BEGIN
+  BLOCK "StringFileInfo"
+  BEGIN
+    BLOCK "040904E4"
+    BEGIN
+      VALUE "a", "bc"
+    END
+  END
+  BLOCK "StringFileInfo"
+  BEGIN
+    BLOCK "040904E4"
+    BEGIN
+      VALUE "a", "bc"
+    END
+    BLOCK "040904E5"
+    BEGIN
+      VALUE "a", "bc"
+    END
+  END
+END

test/tools/llvm-rc/versioninfo-padding.test

+; RUN: llvm-rc /FO %t %p/Inputs/versioninfo-padding.rc
+; RUN: llvm-readobj %t | FileCheck %s
+
+; CHECK:      Resource type (int): 16
+; CHECK-NEXT: Resource name (int): 1
+; CHECK-NEXT: Data version: 0
+; CHECK-NEXT: Memory flags: 0x30
+; CHECK-NEXT: Language ID: 1033
+; CHECK-NEXT: Version (major): 0
+; CHECK-NEXT: Version (minor): 0
+; CHECK-NEXT: Characteristics: 0
+; CHECK-NEXT: Data size: 294
+; CHECK-NEXT: Data: (
+; CHECK-NEXT:   0000: 26013400 00005600 53005F00 56004500  |&.4...V.S._.V.E.|
+; CHECK-NEXT:   0010: 52005300 49004F00 4E005F00 49004E00  |R.S.I.O.N._.I.N.|
+; CHECK-NEXT:   0020: 46004F00 00000000 BD04EFFE 00000100  |F.O.............|
+; CHECK-NEXT:   0030: 00000400 00000000 00000400 00000000  |................|
+; CHECK-NEXT:   0040: 00000000 00000000 04000000 02000000  |................|
+; CHECK-NEXT:   0050: 00000000 00000000 00000000 4E000000  |............N...|
+; CHECK-NEXT:   0060: 01005300 74007200 69006E00 67004600  |..S.t.r.i.n.g.F.|
+; CHECK-NEXT:   0070: 69006C00 65004900 6E006600 6F000000  |i.l.e.I.n.f.o...|
+; CHECK-NEXT:   0080: 2A000000 01003000 34003000 39003000  |*.....0.4.0.9.0.|
+; CHECK-NEXT:   0090: 34004500 34000000 12000300 01006100  |4.E.4.........a.|
+; CHECK-NEXT:   00A0: 00000000 62006300 00000000 7A000000  |....b.c.....z...|
+; CHECK-NEXT:   00B0: 01005300 74007200 69006E00 67004600  |..S.t.r.i.n.g.F.|
+; CHECK-NEXT:   00C0: 69006C00 65004900 6E006600 6F000000  |i.l.e.I.n.f.o...|
+; CHECK-NEXT:   00D0: 2A000000 01003000 34003000 39003000  |*.....0.4.0.9.0.|
+; CHECK-NEXT:   00E0: 34004500 34000000 12000300 01006100  |4.E.4.........a.|
+; CHECK-NEXT:   00F0: 00000000 62006300 00000000 2A000000  |....b.c.....*...|
+; CHECK-NEXT:   0100: 01003000 34003000 39003000 34004500  |..0.4.0.9.0.4.E.|
+; CHECK-NEXT:   0110: 35000000 12000300 01006100 00000000  |5.........a.....|
+; CHECK-NEXT:   0120: 62006300 0000                        |b.c...|
+; CHECK-NEXT: )

tools/llvm-rc/ResourceFileWriter.cpp

 
 // --- VersionInfoResourceResource helpers. --- //
 
 Error ResourceFileWriter::writeVersionInfoBlock(const VersionInfoBlock &Blk) {
   // Output the header if the block has name.
   bool OutputHeader = Blk.Name != "";
   uint64_t LengthLoc;
 
+  padStream(sizeof(uint32_t));
   if (OutputHeader) {
     LengthLoc = writeInt<uint16_t>(0);
     writeInt<uint16_t>(0);
     writeInt<uint16_t>(1); // true
     RETURN_IF_ERROR(writeCString(Blk.Name));
     padStream(sizeof(uint32_t));
   }
 

     RETURN_IF_ERROR(writeVersionInfoValue(*ValuePtr));
   }
 
   if (OutputHeader) {
     uint64_t CurLoc = tell();
     writeObjectAt(ulittle16_t(CurLoc - LengthLoc), LengthLoc);
   }
 
-  padStream(sizeof(uint32_t));
   return Error::success();
 }
 
 Error ResourceFileWriter::writeVersionInfoValue(const VersionInfoValue &Val) {
   // rc has a peculiar algorithm to output VERSIONINFO VALUEs. Each VALUE
   // is a mapping from the key (string) to the value (a sequence of ints or
   // a sequence of strings).
   //

   for (auto &Item : Val.Values)
     (Item.isInt() ? HasInts : HasStrings) = true;
 
   assert((HasStrings || HasInts) && "VALUE must have at least one argument");
   if (HasStrings && HasInts)
     return createError(Twine("VALUE ") + Val.Key +
                        " cannot contain both strings and integers");
 
+  padStream(sizeof(uint32_t));
   auto LengthLoc = writeInt<uint16_t>(0);
   auto ValLengthLoc = writeInt<uint16_t>(0);
   writeInt<uint16_t>(HasStrings);
   RETURN_IF_ERROR(writeCString(Val.Key));
   padStream(sizeof(uint32_t));
 
   auto DataLoc = tell();
   for (size_t Id = 0; Id < Val.Values.size(); ++Id) {

   auto CurLoc = tell();
   auto ValueLength = CurLoc - DataLoc;
   if (HasStrings) {
     assert(ValueLength % 2 == 0);
     ValueLength /= 2;
   }
   writeObjectAt(ulittle16_t(CurLoc - LengthLoc), LengthLoc);
   writeObjectAt(ulittle16_t(ValueLength), ValLengthLoc);
-  padStream(sizeof(uint32_t));
   return Error::success();
 }
 
 template <typename Ty>
 static Ty getWithDefault(const StringMap<Ty> &Map, StringRef Key,
                          const Ty &Default) {
   auto Iter = Map.find(Key);
   if (Iter != Map.end())