[PATCH] D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS
James Henderson via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Jul 18 01:55:22 PDT 2023
jhenderson added a comment.
My apologies, this patch fell off my review queue somehow.
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.
================
Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:16
+
+## Test OBJECT_MODE environment variable when adding symbol table
+# RUN: env OBJECT_MODE= llvm-ranlib t_X32.a
----------------
================
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
----------------
Doesn't this line want an llvm-nm check after it, like the others?
================
Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:31
+
+## Test -X option when adding symbol table.
+# RUN: llvm-ranlib -X32 t_X32.a
----------------
================
Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:48
+
+## Test -X option will override the "OBJECT_MODE" environment variable.
+# RUN: env OBJECT_MODE=32_64 llvm-ranlib -X32 t_X32.a
----------------
================
Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:55
+
+## Test invalid -X option and invalid OBJECT_MODE
+# RUN: not env OBJECT_MODE=any llvm-ranlib t_X64.a 2>&1 | FileCheck --implicit-check-not="error:" --check-prefixes=INVALID-OBJECT-MODE %s
----------------
It would be better to move this block of test cases below the check patterns that are used for the valid cases.
================
Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:62
+
+#GLOB32: var_0x1DF in t32_1.o
+#GLOB32-NEXT: array_0x1DF in t32_1.o
----------------
Nit: here and throughout - check patterns usually have a space between '#' and the pattern name, e.g. "# GLOB32:"
Rather than the somewhat confusing GL64 versus GLOB64 (and similiarly for the 32-bit case), how about "HAS64" and "NO64" (and "HAS32" and "NO32")? That being said, see my comment below about ordering...
================
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
----------------
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.
================
Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:82-85
+#GLOB1: var_0x1DF in t32_1.o
+#GLOB1-NEXT: array_0x1DF in t32_1.o
+#GLOB1-NEXT: var_0x1F7 in t64_1.o
+#GLOB1-NEXT: array_0x1F7 in t64_1.o
----------------
These aren't used. Delete them.
================
Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:87
+
+#INVALID-OBJECT-MODE: error: The OBJECT_MODE environment variable has an invalid setting. OBJECT_MODE must be 32, 64, or 32_64.
+#INVALID-X-OPTION: error: The specified object mode is not valid. Specify -X32, -X64, or -X32_64.
----------------
"value" would be better than "setting" (environment variables have values, not settings).
Please also re-review the coding standards regarding error messages and fix the two separate issues in this and the below error message.
================
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";
}
----------------
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...).
================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1413
+
+ bool HasAixXOption = false;
for (int i = 1; i < argc; ++i) {
----------------
"AIX" looks like it is usually written all-upper-case, so the variable name should be "HasAIXXOption".
================
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
----------------
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.
================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1090-1091
+
+ // If BigArchive, if there is 32-bit globol symbol table, we still use -X64
+ // to generate the 64-bit global symbol table. Same as -X32 option.
+ if (OldArchive->kind() == object::Archive::K_AIXBIG) {
----------------
DiggerLin wrote:
> jhenderson wrote:
> > Just to make sure I'm following what this is trying to say: for Big Archives, a -X64 specification will ignore the 32-bit symbol table and generate a 64-bit one (the 32-bit one will remain in place, if present), and using -X32 will cause a 32-bit symbol table to be generated, right?
> >
> > Assuming that's the case, I suggest the following as the comment:
> >
> > "For archives in the Big Archive format, the bit mode option specifies which symbol table to generate. The presence of a symbol table that does not match the specified bit mode does not prevent creation of the symbol table that has been requested."
> yes, you are correct. I change the comment to your suggest.
The comment doesn't seem to have been changed to my suggestion.
================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:80-81
+ << " -U - Use actual timestamps and uids/gids\n"
+ << " -X(32|64|32_64) - Specific the bit_mode of golbal symbol "
+ "table generated (AIX OS only)\n";
}
----------------
jhenderson wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > According to the AIX ranlib docs, this -X option "Specifies the type of object file ranlib should examine." This doesn't really line up with the description you've used here.
> > yes. any suggestion on the description ?
> Well I'd expect it to be similar to the AIX ranlib doc?
>
> Also, please remember to include spaces before opening parentheses in English prose.
> Also, please remember to include spaces before opening parentheses in English prose.
The "English prose" bit was referring to the description, not how the "-X {32|64|32_64}" bit was printed. Feel free to remove the space like you had before in this case (i.e. either "-X{32|64|32_64}" or "-X {32|64|32_64}" or acceptable).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142660/new/
https://reviews.llvm.org/D142660
More information about the cfe-commits
mailing list