DiggerLin marked 14 inline comments as done.
DiggerLin added a comment.
In D142660#4509464 <https://reviews.llvm.org/D142660#4509464>, @jhenderson
wrote:
> I still have concerns about the option-parsing logic being duplicated, but
> I'm out of time to review it now. I'll try to look tomorrow.
In AIX OS , `nm` and `ar` , do not accept invalid env var OBJECT_MODE, for
example, I will create two patch to fix the behavior of llvm-nm and llvm-ar
(they look invalid env var OBJECT_MODE as default 32bit)
bash-5.0$ env OBJECT_MODE=31 nm d_default.o
0654-216 nm: The OBJECT_MODE environment variable has an invalid setting.
OBJECT_MODE must be 32, 64, 32_64, d64 or any.
-bash-5.0$ env OBJECT_MODE=31 ar -q tmpk.a d_default.o
ar: 0707-133 The OBJECT_MODE environment variable has an invalid setting.
OBJECT_MODE must be 32, 64, 32_64, d64, or any.
================
Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:17
+## Test OBJECT_MODE environment variable when adding symbol table
+# RUN: env OBJECT_MODE= llvm-ranlib t_X32.a
+# RUN: env OBJECT_MODE=32 llvm-ranlib t_X32.a
----------------
jhenderson wrote:
> Doesn't this line want an llvm-nm check after it, like the others?
yes, thanks
================
Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:67
+
+#GL64-NOT: var_0x1F7 in t64_1.o
+#GL64-NOT: array_0x1F7 in t64_1.o
----------------
jhenderson wrote:
> I'm concerned you're going to have an ordering issue in one of the 64 bit or
> 32 bit "NOT" cases. FileCheck -NOT patterns only check the region between the
> previous non-NOT pattern before the -NOT one up to the next non-NOT pattern
> (or the end of the file). So, as things stand, --check-prefixes=GLOB32,GL64
> will verify that the 32-bit table appears and then no 64-bit table is printed
> AFTER the 32-bit table, but not whether the 64-bit table appears BEFORE the
> 32-bit table. Similarly, --check-prefixes=GLOB64,GL32 will check that the
> 64-bit table is printed and then no 32-bit table is printed after it, but not
> whether the 32-bit table is printed before it.
>
> Given that the names of the symbols are irrelevant to this test case, and
> really all that's interesting is which object they're in (since this test
> isn't about checking that the symbol tables have the correct contents - that
> is the duty of a test that doesn't make use of the -X option), could you
> simplify the input objects to have a single like-named symbol and then simply
> ensure the correct object names appear in the output? You could use
> --implicit-check-not to check that e.g. "in t64" or "in t32" don't appear in
> your output.
>
> If you adopted this approach, you'd have two CHECK lines as e.g. follows:
> ```
> # GLOB32: symbol1 in t32_1.o
> # GLOB32-NEXT: symbol2 in t32_2.o
>
> # GLOB64: symbol1 in t64_1.o
> # GLOB64-NEXT: symbol2 in t64_2.o
> ```
> And you'd have FileCheck lines that look like one of the following:
> ```
> FileCheck --check-prefixes=GLOB32 --implicit-check-not="in t64"
> FileCheck --check-prefixes=GLOB64 --implicit-check-not="in t32"
> FileCheck --check-prefixes=GLOB32,GLOB64
> ```
>
> By simplifying down to one symbol each, you could also pass the symbol name
> in as a yaml2obj -D value and only have one YAML file in the test.
thanks for detail explain, I changed as your suggestion.
================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:74-75
+ << " -X {32|64|32_64} - Specifies which archive symbol tables "
+ "should"
+ "be generated if they do not already exist (AIX OS only)\n";
}
----------------
jhenderson wrote:
> Please reflow this string. That should make it fairly obvious that there's a
> mistake in it too (hint: print the help and spot the missing space...).
thanks
================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:237
static bool Verbose = false; ///< 'v' modifier
-static bool Symtab = true; ///< 's' modifier
+static WriteSymTabType Symtab = true; ///< 's' modifier
static bool Deterministic = true; ///< 'D' and 'U' modifiers
----------------
jhenderson wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > Maybe I'm missing something, but I don't see why you need to make this a
> > > custom type. You already have the BitMode value that you read in from the
> > > environment/command-line, and so you can just use that in conjunction
> > > with the `Symtab` boolean to determine what symbol table to write, surely?
> > the Symtab are use to specify whether the symbol table need to write(for
> > non-AIX). and what the symbol table need to write for AIX OS.
> >
> > there are function like
> >
> > ```
> > writeArchiveToBuffer(ArrayRef<NewArchiveMember> NewMembers,
> > WriteSymTabType WriteSymtab, object::Archive::Kind
> > Kind,
> > bool Deterministic, bool Thin)
> > ```
> > and
> >
> > ```
> > Error writeArchive(StringRef ArcName, ArrayRef<NewArchiveMember> NewMembers,
> > WriteSymTabType WriteSymtab, object::Archive::Kind Kind,
> > bool Deterministic, bool Thin,
> > std::unique_ptr<MemoryBuffer> OldArchiveBuf = nullptr);
> > ```
> >
> > call the function as
> >
> >
> > ```
> > writeArchiveToBuffer(M.second.getMembers(),
> > /*WriteSymtab=*/true,
> > /*Kind=*/object::Archive::K_DARWIN,
> > C.Deterministic,
> > /*Thin=*/false);
> > ```
> >
> > I do not want to change the calling parameter of the /*WriteSymtab=*/true,
> >
> > I introduced a new class WriteSymTabType which has following API
> >
> >
> > ```
> > WriteSymTabType(bool PrintSym) { Value = PrintSym ? Both : None; }
> > void operator=(bool PrintSym) { Value = PrintSym ? Both : None; }
> > operator bool() { return Value != None; }
> > ```
> >
> >
> >
> Having looked at this again, I really don't like the implicit conversion
> semantics of the new class, and I don't see why you can't just extend the
> `writeArchiveToBuffer` etc parameters to pass in a `BitMode` parameter.
without implicit conversion, if I add a new parameter BitMode to
writeArchiveToBuffer, I also need to add the new parameter BitMode to function
writeArchive() and writeArchiveToStream() and there is a lot of place(files)
call the function writeArchive, I need to modify all the caller too.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142660/new/
https://reviews.llvm.org/D142660
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
