[PATCH] D111889: [AIX] Support of Big archive (read)

[Digger Lin via Phabricator via llvm-commits]

DiggerLin marked 32 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/lib/BinaryFormat/Magic.cpp:95
+    break;
+
   case '\177':
----------------
jhenderson wrote:
> DiggerLin wrote:
> > DiggerLin wrote:
> > > Esme wrote:
> > > > With this recognition added, other tools' supports for the big archive are enabled, e.g. `llvm-objdump -a`, do we need to add tests for this?
> > > I  tested  llvm-objdump for  archive file in test case of  the patch 
> > > https://reviews.llvm.org/D112097 
> > > 
> > > llvm/test/tools/llvm-objdump/XCOFF/export_sym_list_ar.test
> > I added a new test  for the llvm-obj -a for big archive
> I think it would be good to have at least one test for each of the binutils with a BigArchive input. It's probably sufficient to add that in a separate patch - I'd prefer we be able to generate these input files from a combination of yaml2obj and llvm-ar, so we will need the writing patch to do so.
I will remove the change from the patch, the change do not need from the patch, it will need on when I will implement the change in the create export list.


================
Comment at: llvm/lib/Object/Archive.cpp:111
+
+  if (Size < sizeof(ArMemHdrType)) {
+    if (Err) {
----------------
jhenderson wrote:
> If I'm reading this rightly, this entire block could be moved into common code with the regular archive vareity. If it's not possible to put it into the base class constructor, put it in a helper function or method instead. 
 it can not put into the base class constructor , for the definition of ArMemHdrType is different in the BigArchiveMemberHeader and ArchiveMemberHeader.


================
Comment at: llvm/lib/Object/Archive.cpp:268-271
+  // The raw name itself can be invalid.
+  Expected<StringRef> NameOrErr = getRawName();
+  if (!NameOrErr)
+    return NameOrErr.takeError();
----------------
jhenderson wrote:
> At the moment, this should be a `cantFail`, since no error can actually get here, but see also my above comment re. validation of the raw name.
after address the comment, the getRawName , maybe return a Error.


================
Comment at: llvm/lib/Object/Archive.cpp:342-352
+uint64_t ArchiveMemberHeader::getOffset() const {
+  uint64_t Offset =
+      reinterpret_cast<const char *>(ArMemHdr) - Parent->getData().data();
+  return Offset;
+}
+
+uint64_t BigArchiveMemberHeader::getOffset() const {
----------------
jhenderson wrote:
> These two functions are identical. Could they be pushed into the base class? You could pass in the pointer to get the offset from.
it can not pushed into the base class and pass in the pointer to get offset from.

When you call from the Child  , for example
Header->getOffset() 
Header is AbstractArchiveMemberHeader, it do not have ArMemHdr


================
Comment at: llvm/lib/Object/Archive.cpp:354-363
+// This gets the raw access mode from the ArMemHdr->AccessMode field.
+StringRef ArchiveMemberHeader::getRawAccessMode() const {
+  return StringRef(ArMemHdr->AccessMode, sizeof(ArMemHdr->AccessMode))
+      .rtrim(' ');
+}
+
+StringRef BigArchiveMemberHeader::getRawAccessMode() const {
----------------
jhenderson wrote:
> Same as above. Don't duplicate logic - use functions! In this case, a function that takes an AccessMode field (whatever the type of that field is) should be plenty sufficient.
the ArMemHdr are defined different in the ArchiveMemberHeader and BigArchiveMemberHeader to use a function to take the raw string of AccessMode.


================
Comment at: llvm/lib/Object/Archive.cpp:379-387
+StringRef ArchiveMemberHeader::getRawLastModified() const {
+  return StringRef(ArMemHdr->LastModified, sizeof(ArMemHdr->LastModified))
+      .rtrim(' ');
+}
+
+StringRef BigArchiveMemberHeader::getRawLastModified() const {
+  return StringRef(ArMemHdr->LastModified, sizeof(ArMemHdr->LastModified))
----------------
jhenderson wrote:
> As above - pull into a common function. This looks basically identical to the getRawAccessMode function, so you should be able to do something to avoid duplicating the logic (maybe use a template for the LastModified's/AccessMode's type if needed.
I have think over to use a common function before.   but
for the definition of ArMemHdr is different in ArchiveMemberHeader and BigArchiveMemberHeader , it is difficult to put into a common. 

a lot of duplicated code are caused by the same reason.
If change the name of ArMemHdr  in the BigArchiveMemberHeader  to "BigArMemHdr " , it maybe be more easy to understand the reason.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111889