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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/Archive.h:409
   std::string MergedGloSymTblBuf;
+  bool Has32BitGlobSymTbl = false;
+  bool Has64BitGlobSymTbl = false;
----------------
As commented in other patches, I'd not abbreviate "Global" to "Glob". Just use "Global" in your variable and function names.


================
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
----------------
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?


================
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) {
----------------
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."


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1094
+      BigArchive *BigArc = dyn_cast<BigArchive>(OldArchive);
+      assert(BigArc != nullptr && "BigArc must not nullptr.");
+      if (BigArc->has32BitGlobSymTbl() &&
----------------
Personally, I'd say that this assert is unnecessary, but if you want to keep it, that's fine.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1421
+  // specified.
+  if (object::Archive::getDefaultKindForHost() == object::Archive::K_AIXBIG) {
+    BitMode = getBitMode(getenv("OBJECT_MODE"));
----------------
I think you can share this code with the llvm-ar version of the code (put it in a function). You can either add an "AcceptAny" member to that function's argument, or simply check if the value is "Any" after the function here.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1423
+    BitMode = getBitMode(getenv("OBJECT_MODE"));
+    // -X option in ranlib do not accept "any"
+    if (BitMode == BitModeTy::Unknown || BitMode == BitModeTy::Any)
----------------
Does it treat "any" as "32" or does it report an error? If the latter, should we not do the same here?


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1446
+        } else if (arg.front() == 'X') {
+          if (object::Archive::getDefaultKindForHost() ==
+              object::Archive::K_AIXBIG) {
----------------
Similar to above, could we refactor things slightly to share the code for parsing the -X option between llvm-ranlib and llvm-ar?


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1453
+              fail(Twine("invalid bit mode: ") + arg);
+
+          } else {
----------------
Delete this blank line.


================
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";
 }
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142660/new/

https://reviews.llvm.org/D142660