DiggerLin added inline comments.
================
Comment at: llvm/include/llvm/Object/Archive.h:107
+
+// Define file member header of aix big archive.
+class BigArchiveMemberHeader : public AbstractArchiveMemberHeader {
----------------
jhenderson wrote:
> If I'm not mistaken, "aix" is usually written as "AIX", so we should do the same here (I may easily be wrong though).
thanks
================
Comment at: llvm/include/llvm/Object/Archive.h:65
+
+ Archive const *Parent;
+ AbstractArchiveMemberHeader(const Archive *Parent) : Parent(Parent){};
----------------
jhenderson wrote:
> jhenderson wrote:
> > Please move data to one place, rather than sandwiched between class methods.
> >
> > Also, does this need to be public? It wasn't before...
> > Also, does this need to be public? It wasn't before...
>
> This bit hasn't been addressed.
from the https://llvm.org/doxygen/Archive_8cpp_source.html line 361
uint64_t Size = Header.getSizeOf();
it should be public.
> It wasn't before..
the getSizeOf() is public too in the original code.
================
Comment at: llvm/lib/Object/Archive.cpp:56
+ uint64_t Size, Error *Err) {
+ ErrorAsOutParameter ErrAsOutParam(Err);
+ if (Err) {
----------------
jhenderson wrote:
> Is this safe and correct given that this has been done at the start of the calling code too?
I can not get the comment , I am appreciate that if you can you explain more detail on it.
================
Comment at: llvm/lib/Object/Archive.cpp:58-59
+ if (Err) {
+ std::string Msg("remaining size of archive too small for next archive "
+ "member header ");
+ Expected<StringRef> NameOrErr = ArcMemHeader->getName(Size);
----------------
jhenderson wrote:
> Make this a StringRef (or event a Twine I believe would work), rather than `std::string`.
thanks
================
Comment at: llvm/lib/Object/Archive.cpp:61-66
+ if (!NameOrErr) {
+ consumeError(NameOrErr.takeError());
+ uint64_t Offset = RawHeaderPtr - ArcMemHeader->Parent->getData().data();
+ *Err = malformedError(Msg + "at offset " + Twine(Offset));
+ } else
+ *Err = malformedError(Msg + "for " + *NameOrErr);
----------------
jhenderson wrote:
> I believe if you flip these around, you can do the suggested inline edit. Also note I added the braces for the old else (now the new if part), as I believe the consensus is that if you use braces for an if, you should for all its corresponding else parts too (and vice versa).
thanks
================
Comment at: llvm/lib/Object/Archive.cpp:82
if (Size < sizeof(ArMemHdrType)) {
- if (Err) {
- std::string Msg("remaining size of archive too small for next archive "
- "member header ");
- Expected<StringRef> NameOrErr = getName(Size);
- if (!NameOrErr) {
- consumeError(NameOrErr.takeError());
- uint64_t Offset = RawHeaderPtr - Parent->getData().data();
- *Err = malformedError(Msg + "at offset " + Twine(Offset));
- } else
- *Err = malformedError(Msg + "for " + NameOrErr.get());
- }
+ if (Err)
+ GenerateMemberHeaderParseError(this, RawHeaderPtr, Size, Err);
----------------
jhenderson wrote:
> I don't believe you need this `if` anymore, right?
yes, there is if (Err) check in the createMemberHeaderParseError
================
Comment at: llvm/lib/Object/Archive.cpp:745-750
// Make sure Format is initialized before any call to
- // ArchiveMemberHeader::getName() is made. This could be a valid empty
- // archive which is the same in all formats. So claiming it to be gnu to is
- // fine if not totally correct before we look for a string table or table of
- // contents.
+ // ArchiveMemberHeader::getName() is made. Read Magic to deal with AIX Big
+ // Archive. Else, this could be a valid empty archive which is the same in all
+ // formats. So claiming it to be gnu to is fine if not totally correct before
+ // we look for a string table or table of contents.
Format = K_GNU;
----------------
jhenderson wrote:
> Stare at this code a minute and see if you can spot the bug... (hint, what is the value of `Format` before and after if this is a BigArchive?)
```
} else if (Buffer.startswith(BigMagic)) {
Format = K_AIXBIG;
IsThin = false;
return;
}
```
for BigArchive, there is "return;" in the BigArchive, it will not come here.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111889/new/
https://reviews.llvm.org/D111889