DiggerLin added inline comments.
================
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=32 llvm-ranlib t_X32.a
----------------
stephenpeckham wrote:
> what about OBJECT_MODE= (defined, but empty value)
I added the test case , AIX ranlib treats OBJECT_MODE= (defined, but empty
value) as 32-bit object mode, I implement llvm-ranlib same AIX ranlib
================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:80
+ << " -U - Use actual timestamps and uids/gids\n"
+ << " -X {32|64|32_64} - Specifies the type of object files"
+ "llvm-ranlib should examine (AIX OS only)\n";
----------------
stephenpeckham wrote:
> I think the AIX documentation for ranlib isn't as helpful as it could be. I
> actually like a variation of the original message better:
>
> "-X {32|64|32_64} - Specifies which archive symbol tables should be
> generated if they do not already exist (AIX OS only)\n"
>
> This implies that a 32-bit (64-bit) global symbol table is generated by
> examining XCOFF32 (XCOFF64) members.
>
> But this wording doesn't really fit with the command description: Generate an
> //index// for archives. Should this be "Generate an index or symbol tables
> for archives"? Or just "Generate symbol tables for archives"? The usage
> message for llvm-ar also mixes "index" and "symbol table"
I think the llvm-ranlib generates the global symbol table, index is to general.
if we want to change the description, Maybe the "Generate symbol tables for
archives" is better and we should create a separate patch for it. what do you
think.@jhenderson
================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:127
[P] - use full names when matching (implied for thin archives)
[s] - create an archive index (cf. ranlib)
[S] - do not build a symbol table
----------------
stephenpeckham wrote:
> "Index" or "symbol table"? See the related comment about the usage message
> for "ranlib".
if we want to change to "symbol table" , we need a separate NFC for it. thanks
================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1278
return StringSwitch<BitModeTy>(RawBitMode)
+ .Case("", BitModeTy::Bit32)
.Case("32", BitModeTy::Bit32)
----------------
stephenpeckham wrote:
> AIX commands differentiate between OBJECT_MODE='' (an empty string) and
> OBJECT_MODE not defined. This function treats them the same way.
>
> -X '' (an empty string) should also be an error. I would return Unknown for
> case "". For the Default case, if RawBitMode is NULL, Bit32 should be
> returned.
>
> AIX commands differentiate between OBJECT_MODE='' (an empty string) and
> OBJECT_MODE not defined. This function treats them the same way.
I tried AIX ranlib command , it looks both OBJECT_MODE='' (an empty string) and
"OBJECT_MODE not defined" as 32-bit by default.
> X '' (an empty string) should also be an error. I would return Unknown for
> case "".
I will fix it , thanks
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
