[Object][NFC] Factor out computeHeadersSize.

[jacek]

Summary

In preparation for COFF archives support.

[mstorsjo]

I didn't follow the actual refactoring here yet (it seems at least somewhat nontrivial) - but if this is indeed a pure refactoring, add NFC ("no functional change") as marker to the end of the commit message; in principle, commits are either NFC (with no changes to tests), or if there's a functional change, it should also have a matching test update.

[mstorsjo]

In D143537#4113938, @mstorsjo wrote:

I didn't follow the actual refactoring here yet (it seems at least somewhat nontrivial) - but if this is indeed a pure refactoring, add NFC ("no functional change") as marker to the end of the commit message; in principle, commits are either NFC (with no changes to tests), or if there's a functional change, it should also have a matching test update.

Oh, sorry, I somehow entirely missed that this patch already had the NFC tag. Sorry for the noise...

[MaskRay]

The computeSymbolTableSize change is reasonable. Other changes increase the number of lines. I do not look at follow-ups so cannot tell whether this is reasonable.

llvm/lib/Object/ArchiveWriter.cpp

416

return strlen("!<arch>\n") + computeSymbolTableHeaderSize() + SymtabSize;

The compiler will optimize out strlen. Avoid trivially-used-once variables.

[jacek]

In D143537#4114220, @MaskRay wrote:

The computeSymbolTableSize change is reasonable. Other changes increase the number of lines. I do not look at follow-ups so cannot tell whether this is reasonable.

I updated the diff with suggested change. I need separated HeaderSize in later patches to avoid calling computeSymbolTableHeaderSize multiple times, but I moved adding variables later in the series.

Other changes are mostly meant to pass objects' offsets to writeSymbolTable instead of computing it there from scratch. That computation is trivial now, but it becomes more tricky with later patches, as it would need to take second symbol table, string table and EC symbols table into account (both in writeSymbolTable and in new functions writing additional tables).

llvm/lib/Object/ArchiveWriter.cpp

416

I may change it here, but then I'd need to introduce them in later patches. HeaderSize is useful, because it avoids calling computeSymbolTableHeaderSize up to 3 times, when we have another headers for the second symbol table and EC symbols.

[efriedma]

llvm/lib/Object/ArchiveWriter.cpp

695–696

It's not obvious to me what this constant 8 was supposed to mean, or why it's changing to zero.

707

Please use Optional<uint64_t> to clarify the intent here.

[jacek]

llvm/lib/Object/ArchiveWriter.cpp

695–696

8 meant strlen("!<arch>\n"), I changed it to 0 because computeHeadersSize now takes care of it. I will change bug archive code path to be more similar to other formats, I think it will make it easier to follow.

[jacek]

Thanks for review. The new version uses llvm::Optional and changes big archive code path to be slightly more similar to other formats.

[mstorsjo]

FYI, most (all?) use of llvm::Optional has been migrated to std::optional these days.

[jacek]

In D143537#4213645, @mstorsjo wrote:

FYI, most (all?) use of llvm::Optional has been migrated to std::optional these days.

Thanks, I wasn't sure which one I should use. I will update the patch.

[jacek]

Using std::optional instead of llvm::Optional.

[efriedma]

LGTM

llvm/lib/Object/ArchiveWriter.cpp

static void printNBits(raw_ostream &Out, object::Archive::Kind Kind,
   if (is64BitKind(Kind))
     print<uint64_t>(Out, Kind, Val);
   else
     print<uint32_t>(Out, Kind, Val);
 }
 
 static uint64_t computeSymbolTableSize(object::Archive::Kind Kind,
                                        uint64_t NumSyms, uint64_t OffsetSize,
-                                       StringRef StringTable,
+                                       uint64_t StringTableSize,
                                        uint32_t *Padding = nullptr) {
   assert((OffsetSize == 4 || OffsetSize == 8) && "Unsupported OffsetSize");
   uint64_t Size = OffsetSize; // Number of entries
   if (isBSDLike(Kind))
     Size += NumSyms * OffsetSize * 2; // Table
   else
     Size += NumSyms * OffsetSize; // Table
   if (isBSDLike(Kind))
     Size += OffsetSize; // byte count
-  Size += StringTable.size();
+  Size += StringTableSize;
   // ld64 expects the members to be 8-byte aligned for 64-bit content and at
   // least 4-byte aligned for 32-bit content.  Opt for the larger encoding
   // uniformly.
   // We do this for all bsd formats because it simplifies aligning members.
   // For the big archive format, the symbol table is the last member, so there
   // is no need to align.
   uint32_t Pad = isAIXBigArchive(Kind)
                      ? 0
if (isBSDLike(Kind)) {
     printBigArchiveMemberHeader(Out, "", now(Deterministic), 0, 0,
                                 0, Size, PrevMemberOffset, 0);
   } else {
     const char *Name = is64BitKind(Kind) ? "/SYM64" : "";
     printGNUSmallMemberHeader(Out, Name, now(Deterministic), 0, 0, 0, Size);
   }
 }
 
+static uint64_t computeHeadersSize(object::Archive::Kind Kind, uint64_t NumSyms,
+                                   uint64_t SymNamesSize) {
+  uint32_t OffsetSize = is64BitKind(Kind) ? 8 : 4;
+  uint64_t SymtabSize =
+      computeSymbolTableSize(Kind, NumSyms, OffsetSize, SymNamesSize);
+  auto computeSymbolTableHeaderSize = [=] {
+    SmallString<0> TmpBuf;
+    raw_svector_ostream Tmp(TmpBuf);
+    writeSymbolTableHeader(Tmp, Kind, true, SymtabSize);
+    return TmpBuf.size();
+  };
+
+  return strlen("!<arch>\n") + computeSymbolTableHeaderSize() + SymtabSize;
+}
+
 static void writeSymbolTable(raw_ostream &Out, object::Archive::Kind Kind,
                              bool Deterministic, ArrayRef<MemberData> Members,
-                             StringRef StringTable,
+                             StringRef StringTable, uint64_t MembersOffset,
                              uint64_t PrevMemberOffset = 0) {
   // We don't write a symbol table on an archive with no members -- except on
   // Darwin, where the linker will abort unless the archive has a symbol table.
   if (StringTable.empty() && !isDarwin(Kind))
     return;
 
   unsigned NumSyms = 0;
   for (const MemberData &M : Members)
     NumSyms += M.Symbols.size();
 
   uint64_t OffsetSize = is64BitKind(Kind) ? 8 : 4;
   uint32_t Pad;
-  uint64_t Size = computeSymbolTableSize(Kind, NumSyms, OffsetSize, StringTable, &Pad);
+  uint64_t Size = computeSymbolTableSize(Kind, NumSyms, OffsetSize,
+                                         StringTable.size(), &Pad);
   writeSymbolTableHeader(Out, Kind, Deterministic, Size, PrevMemberOffset);
 
-  uint64_t Pos = isAIXBigArchive(Kind) ? sizeof(object::BigArchive::FixLenHdr)
-                                       : Out.tell() + Size;
-
   if (isBSDLike(Kind))
     printNBits(Out, Kind, NumSyms * 2 * OffsetSize);
   else
     printNBits(Out, Kind, NumSyms);
 
+  uint64_t Pos = MembersOffset;
   for (const MemberData &M : Members) {
     for (unsigned StringOffset : M.Symbols) {
       if (isBSDLike(Kind))
         printNBits(Out, Kind, StringOffset);
       printNBits(Out, Kind, Pos); // member offset
     }
     Pos += M.Header.size() + M.Data.size() + M.Padding.size();
   }
Expected<std::vector<MemberData>> DataOrErr =
                         WriteSymtab, NewMembers);
   if (Error E = DataOrErr.takeError())
     return E;
   std::vector<MemberData> &Data = *DataOrErr;
 
   if (!StringTableBuf.empty() && !isAIXBigArchive(Kind))
     Data.insert(Data.begin(), computeStringTable(StringTableBuf));
 
   // We would like to detect if we need to switch to a 64-bit symbol table.
-  uint64_t LastMemberEndOffset =
-      isAIXBigArchive(Kind) ? sizeof(object::BigArchive::FixLenHdr) : 8;
-  uint64_t LastMemberHeaderOffset = LastMemberEndOffset;
+  uint64_t LastMemberEndOffset = 0;
+  uint64_t LastMemberHeaderOffset = 0;
   uint64_t NumSyms = 0;
   for (const auto &M : Data) {
     // Record the start of the member's offset
     LastMemberHeaderOffset = LastMemberEndOffset;
     // Account for the size of each part associated with the member.
     LastMemberEndOffset += M.Header.size() + M.Data.size() + M.Padding.size();
     NumSyms += M.Symbols.size();
   }
 
+  std::optional<uint64_t> HeadersSize;
+
   // The symbol table is put at the end of the big archive file. The symbol
   // table is at the start of the archive file for other archive formats.
-  if (WriteSymtab && !isAIXBigArchive(Kind)) {
+  if (WriteSymtab && !is64BitKind(Kind)) {
     // We assume 32-bit offsets to see if 32-bit symbols are possible or not.
-    uint64_t SymtabSize = computeSymbolTableSize(Kind, NumSyms, 4, SymNamesBuf);
-    auto computeSymbolTableHeaderSize =
-        [=] {
-          SmallString<0> TmpBuf;
-          raw_svector_ostream Tmp(TmpBuf);
-          writeSymbolTableHeader(Tmp, Kind, Deterministic, SymtabSize);
-          return TmpBuf.size();
-        };
-    LastMemberHeaderOffset += computeSymbolTableHeaderSize() + SymtabSize;
+    HeadersSize = computeHeadersSize(Kind, NumSyms, SymNamesBuf.size());
 
     // The SYM64 format is used when an archive's member offsets are larger than
     // 32-bits can hold. The need for this shift in format is detected by
     // writeArchive. To test this we need to generate a file with a member that
     // has an offset larger than 32-bits but this demands a very slow test. To
     // speed the test up we use this environment variable to pretend like the
     // cutoff happens before 32-bits and instead happens at some much smaller
     // value.
     uint64_t Sym64Threshold = 1ULL << 32;
     const char *Sym64Env = std::getenv("SYM64_THRESHOLD");
     if (Sym64Env)
       StringRef(Sym64Env).getAsInteger(10, Sym64Threshold);
 
     // If LastMemberHeaderOffset isn't going to fit in a 32-bit varible we need
     // to switch to 64-bit. Note that the file can be larger than 4GB as long as
     // the last member starts before the 4GB offset.
-    if (LastMemberHeaderOffset >= Sym64Threshold) {
+    if (*HeadersSize + LastMemberHeaderOffset >= Sym64Threshold) {
       if (Kind == object::Archive::K_DARWIN)
         Kind = object::Archive::K_DARWIN64;
       else
         Kind = object::Archive::K_GNU64;
+      HeadersSize.reset();
     }
   }
 
   if (Thin)
     Out << "!<thin>\n";
   else if (isAIXBigArchive(Kind))
     Out << "<bigaf>\n";
   else
     Out << "!<arch>\n";
 
   if (!isAIXBigArchive(Kind)) {
-    if (WriteSymtab)
-      writeSymbolTable(Out, Kind, Deterministic, Data, SymNamesBuf);
+    if (WriteSymtab) {
+      if (!HeadersSize)
+        HeadersSize = computeHeadersSize(Kind, NumSyms, SymNamesBuf.size());
+      writeSymbolTable(Out, Kind, Deterministic, Data, SymNamesBuf,
+                       *HeadersSize);
+    }
     for (const MemberData &M : Data)
       Out << M.Header << M.Data << M.Padding;
   } else {
+    HeadersSize = sizeof(object::BigArchive::FixLenHdr);
+    LastMemberEndOffset += *HeadersSize;
+    LastMemberHeaderOffset += *HeadersSize;
+
     // For the big archive (AIX) format, compute a table of member names and
     // offsets, used in the member table.
     uint64_t MemberTableNameStrTblSize = 0;
     std::vector<size_t> MemberOffsets;
     std::vector<StringRef> MemberNames;
     // Loop across object to find offset and names.
     uint64_t MemberEndOffset = sizeof(object::BigArchive::FixLenHdr);
     for (size_t I = 0, Size = NewMembers.size(); I != Size; ++I) {
if (NewMembers.size()) {
         Out << MemberName << '\0'; // Member file name, null byte padding.
 
       if (MemberTableNameStrTblSize % 2)
         Out << '\0'; // Name table must be tail padded to an even number of
                      // bytes.
 
       if (WriteSymtab && NumSyms > 0)
         writeSymbolTable(Out, Kind, Deterministic, Data, SymNamesBuf,
-                         LastMemberEndOffset);
+                         *HeadersSize, LastMemberEndOffset);
     }
   }
   Out.flush();
   return Error::success();
 }
 
 Error writeArchive(StringRef ArcName, ArrayRef<NewArchiveMember> NewMembers,
                    bool WriteSymtab, object::Archive::Kind Kind,