[PATCH] D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS

[Digger Lin via Phabricator via llvm-commits]

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. at 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