[PATCH] D111889: [AIX] Support of Big archive (read)
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jan 5 02:17:22 PST 2022
jhenderson 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
----------------
DiggerLin wrote:
> 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
No need to change this at this point, but noting that a later patch to rename Archive to UnixArchive or similar, should probably result in renaming `ArchiveMagic` to `UnixArchiveMagic`.
================
Comment at: llvm/include/llvm/Object/Archive.h:81
+template <typename T>
+class CommonMemHeaderFieldAPI : public AbstractArchiveMemberHeader {
+public:
----------------
DiggerLin wrote:
> DiggerLin wrote:
> > 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
> >
> >
> >
> please add ; after the definition of class of above code
Fair point about the compilation issue. Maybe we should just move the `A` classes out of their parent classes? I'm not sure the nesting really gives us that much, and it seems to be making things more complex. What do you think?
================
Comment at: llvm/include/llvm/Object/Archive.h:115
+ Expected<const char *> getNextChildLoc() const override;
+ virtual Expected<bool> isThin() const override;
----------------
DiggerLin wrote:
> 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();
> ...}
>
I'm not sure what code you're referencing here, but it's irrelevant. `virtual` is "inherited" from overridden functions. This `isThin` function is declared to be `virtual` in a base class (as shown by the use of `override`. That means all subclass `isThin` functions will be `virtual` automatically, and don't need to be annotated as such. The use of `override` makes it clear that this function MUST be `virtual`, so the old pre-C++11 practice of marking subclass functions as `virtual` to indicate they are overriding base class functions is no longer necessary.
(If you think you need `virtual` for `isThin`, why don't you need it for e.g. `getName`, `getSize` etc?)
================
Comment at: llvm/include/llvm/Object/Archive.h:414
+ char MemOffset[20]; ///< Offset to member table.
+ char GlobSymOffset[20]; ///< Offset to global symbol table.
+ char
----------------
jhenderson wrote:
> Still need to clang-foramt some of this content, it looks like.
Ping? Linter is still complaining about lack of clang-formatting here.
================
Comment at: llvm/lib/Object/Archive.cpp:203
+
+static uint64_t alignTo2(uint64_t Size) { return (Size + 1) >> 1 << 1; }
+
----------------
`evenAlign` might be a little clearer as to the intent.
================
Comment at: llvm/lib/Object/Archive.cpp:1168
+ if (RawOffset.getAsInteger(10, LastChildOffset))
+ Err = malformedError("malformed aix big archive: last member offset \"" +
+ RawOffset + "\" is not a number");
----------------
Already highlighted before with my above comment with "and below": "aix" -> "AIX"
================
Comment at: llvm/lib/Object/Archive.cpp:424-426
+ // If Size is odd, add 1 to make it even.
+ const char *NextLoc =
+ reinterpret_cast<const char *>(ArMemHdr) + (((Size + 1) >> 1) << 1);
----------------
jhenderson wrote:
> This padding is in a couple of different places now. I think it would be good to move it into a small helper function, called e.g. `makeEven`.
>
> That being said, there are some functions elsewhere within LLVM that align things (see "alignTo"). Could you use that instead. here and in similar places?
> That being said, there are some functions elsewhere within LLVM that align things (see "alignTo"). Could you use that instead. here and in similar places?
Thanks for the helper, but did you look for other LLVM functions?
================
Comment at: llvm/test/Object/archive-big-extract.test:4
+# RUN: llvm-ar x %p/Inputs/aix-big-archive.a
+# RUN: diff %t/extracted/empty.o %p/Inputs/aix-big-archive-member.o
----------------
If `empty.o` is just an empty file, rather than an object file, don't use it here in the diff. Instead, use `touch` or `echo` to create a new file with contents that exactly match those that are expected, and diff using that instead. This will remove one dependency on a canned object.
================
Comment at: llvm/test/Object/archive-big-print.test:2
+## Test printing an archive created by AIX ar (Big Archive).
+# RUN: llvm-ar p %p/Inputs/aix-big-archive.a evenlen | FileCheck %s --check-prefix=AIXBIG --implicit-check-not={{.}}
+# AIXBIG: content_of_evenlen
----------------
You're still unnecessarily using `--check-prefix`. Please fix here and in every other test you're adding.
================
Comment at: llvm/test/Object/archive-big-read.test:1
+## Test reading an archive created by AIX ar (Big Archive).
+RUN: env TZ=GMT llvm-ar tv %p/Inputs/aix-big-archive.a | FileCheck %s --strict-whitespace --check-prefix=AIXBIG --implicit-check-not={{.}}
----------------
jhenderson wrote:
> jhenderson wrote:
> > In the future, the input will hopefully be generated on the fly rather than using a canned binary.
> >
> > Also, add comment markers and don't use check-prefix, as above.
> > Also, add comment markers and don't use check-prefix, as above.
>
> This hasn't been addressed.
This STILL hasn't been addressed, despite being marked as done. Please fix.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111889/new/
https://reviews.llvm.org/D111889
More information about the llvm-commits
mailing list