[PATCH] D144872: [AIX] Align the content of an xcoff object file which has auxiliary header in big archive.

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:452
+      reinterpret_cast<const char *>(&AuxHeader->MaxAlignOfData) -
+          reinterpret_cast<const char *>(AuxHeader) + 2)
+    return MinBigArchiveMemDataAlign;
----------------
jhenderson wrote:
> I still don't understand specifically why "2". Should this use some form of `offsetof` to get the actual position in the struct or similar?
yes.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:567
   static char PaddingData[8] = {'\n', '\n', '\n', '\n', '\n', '\n', '\n', '\n'};
-
+  static uint64_t MemHeadPadSize = 0;
   uint64_t Pos =
----------------
jhenderson wrote:
> We've had this sort of discussion before: you can't use static local variables like this that will change each time this function is called, because this function could be called from multiple threads.
thanks for let me know.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:636
+
+  for (auto M = NewMembers.begin(); M < NewMembers.end(); M++) {
     std::string Header;
----------------
jhenderson wrote:
> Please re-review the LLVM coding standards. You keep making mistakes, despite being told before, that are covered in the coding standards.
sorry for careless, actually I know that ++M is required by LLVM coding standard. 


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:687-688
+      Pos += MemHeadPadSize;
+      uint64_t NextOffset = Pos + sizeof(object::BigArMemHdrType) +
+                            alignTo(M->MemberName.size(), 2) + alignTo(Size, 2);
+
----------------
jhenderson wrote:
> Is this value correct for the last member in an archive?
yes, the value is correct.  the NextOffset  of last file member will be point to "Member Table" . and there is not a special requirement of  content of "Member Table"


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:695-696
+            getSymbolFile((M + 1)->Buf->getMemBufferRef(), Context);
+        if (!SymFileOrErr)
+          return createFileError((M + 1)->MemberName, SymFileOrErr.takeError());
+
----------------
jhenderson wrote:
> If I'm not mistaken, the implication of this code is that you will no longer be able to store non-symbolic-files in BigArchives. Is that intended (and if so, is it correct)? For other formats, you can store non-symbolic files as archive members. They just don't contribute anything to the symbol table.
in the function

```
static Expected<std::unique_ptr<SymbolicFile>>
getSymbolFile(MemoryBufferRef Buf, LLVMContext &Context) {
  std::unique_ptr<object::SymbolicFile> Obj;
  const file_magic Type = identify_magic(Buf.getBuffer());
  // Treat non symbolic file types as nullptr.
  if (!object::SymbolicFile::isSymbolicFile(Type, &Context))
    return nullptr;
```

it treats non symbolic file types as nullptr. it do not return a error.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:720-724
+        Expected<std::unique_ptr<SymbolicFile>> SymFileOrErr =
+            getSymbolFile(Buf, Context);
+        if (!SymFileOrErr)
+          return createFileError(M->MemberName, SymFileOrErr.takeError());
+        CurSymFile = std::move(*SymFileOrErr);
----------------
jhenderson wrote:
> If I'm not mistaken, you could avoid a lot of this duplicate logic about the SymbolicFile by pulling it out of the isAIXBigArchive code. Something like this outline:
> 
> ```
> for (auto M = NewMembers.begin(); M < NewMembers.end(); ++N) {
>   if (NeedSymbols || isAIXBigArchive(Kind)) {
>     if (M == NewMembers.begin() {
>       CurSymFile = // load symbolic file for first member
>     } else {
>       CurSymFile = std::move(NextSymFile);
>     }
>     if (M + 1 != NewMembers.end()) {
>       NextSymFile = // load symbolic file for next member
>     }
> 
>     // Do all of loop logic
>   }
> }
> ```
> You'll need to handle non-symbolic files somehow, perhaps by just resetting the relevant std::unique_ptr to leave it empty, and then checking whether the pointer is empty before trying to read it.
in the  if (M == NewMembers.begin())  , it not only open CurSymFile but also calculate MemHeadPadSize 

in the if ((M + 1) != NewMembers.end()),  it not only open CurSymFile but also calculate NextMemHeadPadSize but also calculate the NextMemHeadPadSize

even if I do as your suggestion, I still need to  if (M == NewMembers.begin())  and if ((M + 1) != NewMembers.end()) for MemHeadPadSize  and NextMemHeadPadSize later



> You'll need to handle non-symbolic files somehow, perhaps by just resetting the relevant std::unique_ptr to leave it empty, and then checking whether the pointer is empty before trying to read it.
  function getSymbolFile already return nullptr for non-symbolic files.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144872