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

[Digger Lin via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/Object/Archive.h:35-39
+namespace archive {
+const char Magic[] = "!<arch>\n";
+const char ThinMagic[] = "!<thin>\n";
+const char BigMagic[] = "<bigaf>\n";
+} // namespace archive
----------------
jhenderson wrote:
> Rather than putting these in a new archive namespace (and then not putting the rest of the archive code in that namespace...), I'd suggest renaming the variables, and leaving them in the object namespace directly. Suggested names would be "ArchiveMagic", "ThinArchiveMagic", and "BigArchiveMagic" (optionally using "Ar" instead of "Archive", if you want something more succinct).
thanks


================
Comment at: llvm/include/llvm/Object/Archive.h:81
+template <typename T>
+class CommonMemHeaderFieldAPI : public AbstractArchiveMemberHeader {
+public:
----------------
jhenderson wrote:
> Thanks for the class restructuring. I hope you agree that it looks better not having the duplicated code.
> 
> I think the class name needs changing. Apart from anything else, the name says nothing about archives. I also don't think "FieldAPI" makes a huge amount of sense to me (I see what you're trying to do though). I have two possible alternatives:
> # My preferred approach would be to rename `ArchiveMemberHeader` to `UnixArchiveMemberHeader`, and then use `ArchiveMemberHeader` for this class's name.
> # If renaming the `ArchiveMemberHeader` touches too much code, I'd suggest renaming this class to `CommonArchiveMemberHeader`.
> 
> Abbreviating "Archive" to "Ar" and "Member" to "Mem" is okay, in either case.
> 
> Additionally, you haven't got it quite right: the `T` parameter here should be the current `T::ArMemHdrType` instead, with the `ArMemHdr` being passed into this class's constructor (and potentially the instance in the derived clas isn't needed). That'll avoid the need for a) the `friend` declarations (I believe), and b) the need to repeatedly do `const T &DerivedMemberHeader = static_cast<const T &>(*this);` in the get* methods.
if change  ArchiveMemberHeader -> UnixArchiveMemberHeader 
I think we need to change Archive to UnixAchive too at this moment,  changing Archive to UnixAchive will cause a lot of llvm-* file changing.  (I think we need another separate  patch for changing  Archive to UnixAchive later).
I am prefer method 2 now.



> Additionally, you haven't got it quite right: the T parameter here should be the current T::ArMemHdrType instead, with the ArMemHdr being passed into this class's constructor (and potentially the instance in the derived clas isn't needed). That'll avoid the need for a) the friend declarations (I believe), and b) the need to repeatedly do const T &DerivedMemberHeader = static_cast<const T &>(*this); in the get* methods.


I do not think I can do as your suggestion,  for example : the 
ArchiveMemberHeader are not instantiated complete,  ArchiveMemberHeader::ArMemHdrType will be incomplete type.

you can try to compile the code 


```
class AbstractArchiveMemberHeader {
};

template <typename T>
class CommonArchiveMemberHeader : public AbstractArchiveMemberHeader {
 T *a;
}

class ArchiveMemberHeader
    : public CommonArchiveMemberHeader<ArchiveMemberHeader::A> {

 struct A {
  };
}

class BigArchiveMemberHeader
    : public CommonArchiveMemberHeader<BigArchiveMemberHeader::A> {
   struct A {
  };
}
```

you will get error when compile





================
Comment at: llvm/include/llvm/Object/Archive.h:115
+  Expected<const char *> getNextChildLoc() const override;
+  virtual Expected<bool> isThin() const override;
 
----------------
jhenderson wrote:
> No need for `virtual` here. Just delete it.
It  need virtual here 

in the function Archive::Child::Child(const Archive *Parent, const char *Start, Error *Err)
    : Parent(Parent) 
{
....
uint64_t Size = Header->getSizeOf();
...}



================
Comment at: llvm/lib/Object/Archive.cpp:65
+
+#define getFieldRawString(T) StringRef(T, sizeof(T)).rtrim(" ")
+
----------------
jhenderson wrote:
> Continuing our discussion that has now moved too far from the relevant site. As these fields are char arrays, you should be able to do something like:
> ```
> template <class T, std::size_t N>
> StringRef getFieldRawString(const T (&Field)[N]) {
>   return StringRef(Field, N).rtrim(" ");
> }
> ```
> This will populate the template parameter N with the char array size, using template auto-deduction, avoiding the need for the sizeof entirely. See how the `std::size` signature works in C++17 for an example of this (note that obviously we don't want to use `std::size` itself here).
thanks
   


================
Comment at: llvm/lib/Object/Archive.cpp:219
+      StringRef(ArMemHdr->Name, NameLenWithPadding + 2);
+  const char NameTerminator[3] = {0x60, 0x0a, 0x00};
+  if (!NameStringWithNameTerminator.endswith(NameTerminator)) {
----------------
jhenderson wrote:
> Any reason this can't be `StringRef NameTerminator = "`\n`?
> 
> I'd also put it before the `NameStringWithNameTerminator`, and you can then use `NameTerminator.size()` to set the length for `NameStringWithNameTerminator`
thanks


================
Comment at: llvm/lib/Object/Archive.cpp:225-227
+        "name have not name terminator \"0X60\\0X0a\" for archive member "
+        "header at offset " +
+        Twine(Offset));
----------------
jhenderson wrote:
> Could you not use `createMemberHeaderParseError` rather than calling `malformedError` directly here?
I think we present a more specific error information here than using createMemberHeaderParseError.


================
Comment at: llvm/lib/Object/Archive.cpp:681-686
+    return strlen(ThinMagic);
+
+  if (Kind() == K_AIXBIG)
+    return strlen(BigMagic);
+
+  return strlen(Magic);
----------------
jhenderson wrote:
> I believe you can use `sizeof` instead of `strlen` here, as the magic strings are char arrays, rather than pointers. Please double-check though.
use sizeof will have one more bytes than strlen() , for the sizeof also include "\0" in the string.


================
Comment at: llvm/test/Object/archive-big-extract.test:2
+## Test printing an archive created by AIX ar (Big Archive).
+RUN: llvm-ar p %p/Inputs/bigFormat.a evenlen | FileCheck %s --check-prefix=AIXBIG --implicit-check-not={{.}}
+AIXBIG: content_of_evenlen
----------------
jhenderson wrote:
> DiggerLin wrote:
> > DiggerLin wrote:
> > > sfertile wrote:
> > > > I think James is suggesting that the 'extract' in the test name is misplaced since we are not in fact extracting anything. Instead how about naming the test something along the lines of `big-archive-print.test`.
> > > > 
> > > > > I think we should add "x" option in the llvm/test/tools/llvm-ar/extract.test after implement write big archive.
> > > > 
> > > > It is nice to reuse the existing tests for the new format, but as you pointed out we can't until we implement writing big archive files first. If you add an extract test in this directory (or even as a second runstep in this file), with the binary file already being added in this test then we get coverage now, and don't have to wait until we implement big archive writing. 
> > > if I added the -x to extract test the archive file Input/libtest.a (the archive file also is used in https://reviews.llvm.org/D112097 ) in the test case.  I will use "diff command" to diff the extracting object file with original object file. I think I have to add two original object files into directory llvm/test/Object/Input for compare . Adding a extra object file is not a good idea unless we need it. if you strong suggestion I add the -x option with Input/libtest.a. I will add it.
> > I think I can use bigFormat.a for option -x , I will add -x option test. thanks
> As noted already, this test, in its current form, isn't about extracting the archive contents, so it should be renamed, and comments/names updated accordingly, e.g. `archive-big-print.test` (note that the -p option is for printing a member, not extracting it). Adding a -x test case is a tangential thing, which can be added either as part of this test case, or separately, I don't mind.
the test case using option -x  to extract the member file out the archive. and  compare the file content,  why we need to change the name to archive-big-print.test ? 

and  there already has  test name as "archive-big-print.test " in the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111889