[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:848-849
+
+      // If there is next member file. we need to calculate the padding before
+      // the header if there is.
+      if ((M + 1) != NewMembers.end()) {
----------------



================
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:
> > 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).
> >   }
> > }
> > ```
> if I change as your suggestion,  
> it only reduce the code of 
>   
> ```
>  if (!isAIXBigArchive(Kind)) {
>         Expected<std::unique_ptr<SymbolicFile>> SymFileOrErr =
>             getSymbolFile(Buf, Context);
>         if (!SymFileOrErr)
>           return createFileError(M->MemberName, SymFileOrErr.takeError());
>         CurSymFile = std::move(*SymFileOrErr);
>       }
> ```
> 
> I still need to put variables `NextOffset` and `OffsetToMemData`  under the  `if (NeedSymbols || isAIXBigArchive(Kind)) `
> 
> 
> and it will several 
> 
> `if (isAIXBigArchive(Kind)) ` under  `if (NeedSymbols || isAIXBigArchive(Kind)) {`
> 
> 
> we will have duplication code of 
> `printMemberHeader(Out, Pos, StringTable, MemberNames, Kind, Thin, *M, ModTime, Size);`
> 
> 
> ```
>   if (NeedSymbols || isAIXBigArchive(Kind)) {
>     ...
>    if(!isAIXBigArchive(Kind)) 
>      printMemberHeader(Out, Pos, StringTable, MemberNames, Kind, Thin, *M, ModTime, Size);
>  }
>   else {
>       printMemberHeader(Out, Pos, StringTable, MemberNames, Kind, Thin, *M, ModTime, Size);
>    }  
> ```
> 
> I do not think the logic of your suggestion is clearer than the current code. If you strong suggest , I can change as your suggestion.
Either you've misunderstood my suggestion or I've missed something. I'm pretty sure I haven't missed anything, as I've just experimented with reorganising the code locally, and came up with a perfectly reasonable looking solution without even referring back to my old one, yet after looking back at it, it looks fairly similar.

The changes I made were as follows:
```
  for (...) {
    ... after the existing file size limit check ...

    if (NeedSymbols || isAIXBigArchive(Kind)) {
      auto SetNextSymFile = [&NextSymFile,
                             &Context](MemoryBufferRef Buf,
                                       StringRef MemberName) -> Error {
        Expected<std::unique_ptr<SymbolicFile>> SymFileOrErr =
            getSymbolicFile(Buf, Context);
        if (!SymFileOrErr) {
          return createFileError(MemberName, SymFileOrErr.takeError());
        }
        NextSymFile = std::move(*SymFileOrErr);
        return Error::success();
      };

      if (M == NewMembers.begin())
        if (Error Err = SetNextSymFile(Buf, M->MemberName))
          return std::move(Err);
      CurSymFile = std::move(NextSymFile);

      if (M + 1 != NewMembers.end())
        if (Error Err = SetNextSymFile((M + 1)->Buf->getMemBufferRef(), (M + 1)->MemberName))
          return std::move(Err);
    }

    if (isAIXBigArchive(Kind)) {
      uint64_t OffsetToMemData = Pos + sizeof(object::BigArMemHdrType) +
                                 alignTo(M->MemberName.size(), 2);

      if (M == NewMembers.begin()) {
        MemHeadPadSize = alignToPowerOf2(OffsetToMemData,
                                         getMemberAlignment(CurSymFile.get())) -
                         OffsetToMemData;
      } else {
        MemHeadPadSize = NextMemHeadPadSize;
      }

      ... update Pos, calculate NextOffset etc as before, then write header ...
    } else {
      printMemberHeader(...);
    }
    Out.flush();

    std::vector<unsigned> Symbols;
    if (NeedSymbols) {
      Expected<std::vector<unsigned>> SymbolsOrErr =
          getSymbols(CurSymFile.get(), Index, SymNames, SymMap);
      if (!SymbolsOrErr)
        return createFileError(M->MemberName, SymbolsOrErr.takeError());
      Symbols = std::move(*SymbolsOrErr);
      if (CurSymFile) // Can we remove this if check and set HasObject to true where CurSymFile is set?
        HasObject = true;
    }

    ... update Pos and Ret ...
  }
```

I think this logic flows quite nicely, and certainly better than the current suggest. It avoids multiple different call sites to getSymbolicFile, for the cost of a few simple ifs. It also avoids any significant nested ifs (i.e. ones where the inner if is a large block), which helps readability of the code.

Aside: is the check on CurSymFile for the HasObject setting necessary? This potentially could be simplified, but I haven't got the time to follow this logic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144872