[PATCH] D111889: [AIX] Support of Big archive (read)
Digger Lin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 20 10:46:50 PDT 2021
DiggerLin marked 22 inline comments as done.
DiggerLin added inline comments.
================
Comment at: llvm/include/llvm/Object/Archive.h:35
+constexpr int ArchiveMaigcLen = 8;
+
----------------
jhenderson wrote:
> I'd like to get rid of this constant, since it's not guaranteed for all conceptual archive types. Also there's a typo in it: `ArchiveMagicLen`.
>
> See my below comments for how I'd get rid of it.
I think define a "ArchiveMaigcLen = 8" is mandatory here.
if you look at the
https://llvm.org/doxygen/Archive_8cpp_source.html
line 1000
> // Returns true if archive file contains no member file.
> bool Archive::isEmpty() const { return Data.getBufferSize() == 8; }
it use 8 to hardcode the magic length. I think define a const expr is much better than hardcode.
================
Comment at: llvm/include/llvm/Object/Archive.h:129
+ Expected<uint64_t> getNextOffset() const;
+ static bool classof(AbstractArchiveMemberHeader const *v);
+
----------------
jhenderson wrote:
> Alternative names also welcome, but not a single lower-case letter that has no relevance to the thing.
thanks
================
Comment at: llvm/include/llvm/Object/Archive.h:142-145
+ union {
+ char Name[2]; // Start of member name
+ char Terminator[2];
+ };
----------------
jhenderson wrote:
> Why do you need a union here? Could you not simply either use `Name` direectly in the raw header, or call it `NameOrTerminator`? Is this even really a part of the on-disk archive member header?
the definition is come from aix OS /usr/include/ar.h and https://www.ibm.com/docs/en/aix/7.2?topic=formats-ar-file-format-big
I think the definition is not only for read a big archive, it will use for a write big archive to(encode to a big archive).
using a union is more explicitly on what is the member for, it for name or Terminator when encoding a big archive later.
================
Comment at: llvm/include/llvm/Object/Archive.h:151
class Archive : public Binary {
virtual void anchor();
----------------
jhenderson wrote:
> Rather than be a potential concrete type, I think it would be cleaner if `Archive` were an abstract type, with concrete implementations for Big and traditional (better name required) archives. With Archive being a concrete type, there's a risk of slicing when someone thinks they've got a traditional archive, but actually have a BigArchive.
>
> This then allows some somewhat better (in my opinion) locations for certain things.
if order to not modify a lot of tools code, we make Archive an abstract type. This patch is big enough. we can implement a derived class GNUArchive class derived from archive(as your suggestion in EGuesnet's patch) too in another separate patch.
and move the code
> mutable std::vector<std::unique_ptr<MemoryBuffer>> ThinBuffers;
> std::vector<std::unique_ptr<MemoryBuffer>> takeThinBuffers() {
> return std::move(ThinBuffers);
> }
>
>
from class Archive to class GNUArchive the other separate patch.
================
Comment at: llvm/include/llvm/Object/Archive.h:358
uint32_t getNumberOfSymbols() const;
+ virtual uint64_t getFirstChildeOffset() const { return ArchiveMaigcLen; }
----------------
jhenderson wrote:
> Related to my other comment, I think it would make sense for `ArchiveMagicLen` to be inside each concrete class, and returned via a virtual getter.
good Idea, thanks
================
Comment at: llvm/include/llvm/Object/Archive.h:393-394
+ const BigArFixLenHdrType *ArFixLenHdr = nullptr;
+ uint64_t FirstArOffset = 0;
+ uint64_t LastArOffset = 0;
+
----------------
jhenderson wrote:
> The names of the two members don't make sense to me - they seem liks "First Archive offset" which is clearly not what they mean. They need better names. I'm thinking they should actually be `FirstMemberOffset` and `LastMemberOffset` or something similar.
thanks
================
Comment at: llvm/include/llvm/Object/Archive.h:399
+ uint64_t getFirstChildeOffset() const override { return FirstArOffset; }
+ uint64_t getLastArOffset() const { return LastArOffset; }
+};
----------------
jhenderson wrote:
> Same comment as above - rename the function to match. Perhaps `getLastChildOffset` would make the most sense, since it mirrors the other's name.
I change to getLastMemberOffset. thanks
================
Comment at: llvm/lib/Object/Archive.cpp:1195
+ if (RawStringRef.rtrim(' ').getAsInteger(10, FirstArOffset)) {
+ report_fatal_error("Malformed AIX Big Archive: First Archive Offset \"" +
+ RawStringRef + "\" is not a number.");
----------------
jhenderson wrote:
> 1) Ifs that contain only a single statement shouldn't have braces.
> 2) Why is this a report_fatal_error call rather than setting the input `Err` variable?
> 3) Please follow the LLVM standards for error messages (https://llvm.org/docs/CodingStandards.html#error-and-warning-messages).
> 4) Don't capitalize "First Archive Offset". Use either the actual field name as per the spec, or just make it all lower-case.
> 5) Don't use "Archive" here in the "First Archive Offset". Use "member" or "child" instead. The archive doesn't consist of other archives (normally!). I think the spec has used a misleading name, so let's not follow it unless we're using the term as literally stated in the spec (`fl_fstmoff`).
thanks
================
Comment at: llvm/lib/Object/Archive.cpp:1204
+ if (RawStringRef.getAsInteger(10, LastArOffset)) {
+ report_fatal_error("Malformed AIX Big Archive: Last Archive Offset \"" +
+ RawStringRef + "\" is not a number.");
----------------
jhenderson wrote:
> Same comments as above.
thanks
================
Comment at: llvm/test/Object/archive-big-extract.test:1
+## Test extracting 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={{.}}
----------------
jhenderson wrote:
> This isn't "extracting" it - it is "printing" it. The "x" option is extraction of a member (you may wish to have a test case for that option too).
I think we should add "x" option in the llvm/test/tools/llvm-ar/extract.test after implement write big archive.
in the llvm/test/tools/llvm-ar/extract.test , it use following to generate a archive file and then test extract.
> # RUN: echo filea > %t/a.txt
> # RUN: echo fileb > %t/b.txt
> # RUN: llvm-ar rc %t/archive.a %t/a.txt %t/b.txt
>
> # RUN: llvm-ar x %t/archive.a 2>&1 | count 0
> # RUN: diff %t/a.txt %t/extracted/a.txt
> # RUN: diff %t/b.txt %t/extracted/b.txt
================
Comment at: llvm/test/Object/archive-big-read.test:3-5
+AIXBIG: rw-r--r-- 0/0 19 Jan 1 00:00 1970 evenlen
+AIXBIG-NEXT: rw-r--r-- 0/0 7 Jan 1 00:00 1970 oddlen
+AIXBIG-NEXT: rw-r--r-- 0/0 258 Jan 1 00:00 1970 empty.o
----------------
jhenderson wrote:
> I'm guessing the numbers in the column before the date are the sizes, although I don't know. If they are the size fields, the sizes don't match what the names imply they should (i.e. even/odd/zero).
yes , before the date are the sizes. but the file name imply the content of the file, not imply the size.
-bash-5.0$ cat evenlen
content_of_evenlen
-bash-5.0$ cat oddlen
oddlen
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