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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:664-665
 
+    // In big archive, we need to calculate and include next member offset
+    // and previous member offset in file member header.
     if (isAIXBigArchive(Kind)) {
----------------
(and make sure the comment respects the column limit properly - I haven't attempted to with my edit)


================
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);
----------------
DiggerLin wrote:
> 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.
> 
> 
My complaint is that structurally, this and the above block are largely the same. Yes, one does a bit more than the other, but ultimately, the two are structurally identical.

You could refactor the code to avoid this structural duplication, without too much difficulty, using my suggested code above. To do the extra work regarding calculate the padding sizes, simply add "`if (isAIXBigArchive(Kind))` clauses:
```
for (auto M = NewMembers.begin(); M < NewMembers.end(); ++M) {
  if (NeedSymbols || isAIXBigArchive(Kind)) {
    if (M == NewMembers.begin() {
      CurSymFile = // load symbolic file for first member
      if (isAIXBigArchive(Kind)) {
        // Do stuff with padding, as needed.
      }
    } else {
      CurSymFile = std::move(NextSymFile);
    }
    if (M + 1 != NewMembers.end()) {
      NextSymFile = // load symbolic file for next member
      if (isAIXBigArchive(Kind)) {
        // Do stuff with padding, as needed.
      }
    }

    // Do all of loop logic (or do some before the ifs and some after).
  }
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144872