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

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:3
+## Test the -X option.
+## The option specifies the type of object file on which llvm-ranlib will operate. 
+
----------------
Nit: trailing whitespace


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1394
           object::Archive::K_AIXBIG) {
         Match = *(*ArgIt + 2) != '\0' ? *ArgIt + 2 : *(++ArgIt);
         BitMode = getBitMode(Match);
----------------
Unrelated to this patch, but I spotted this when comparing the llvm-ar and llvm-ranlib parsing logic: what happens if -X is the very final argument, without a value? E.g. `llvm-ar (any other args...) -X`?


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1437
+            const char *Xarg = arg.data();
+            if (Xarg[0] == '\0')
+              BitMode = BitModeTy::Unknown;
----------------
If I'm reading this correctly, llvm-ranlib will accept "-X32" but reject "-X 32". Is that intentional? If so, what's the motivation behind it (I would say that there needs to be a motivation other than "that's what AIX ranlib does)?


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1442
+
+            // -X option in ranlib do not accept "any"
+            if (BitMode == BitModeTy::Unknown || BitMode == BitModeTy::Any)
----------------
I know that AIX ranlib doesn't accept "any" whereas I believe AIX ar does. I wonder though what the benefit is of preventing llvm-ranlib from accepting "any"? Dropping that special case would simplify the code.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1463
 
+  if (object::Archive::getDefaultKindForHost() == object::Archive::K_AIXBIG) {
+    // If not specify -X option, get BitMode from enviorment variable
----------------
Is there a particular reason that this is after the command-line option parsing for llvm-ranlib, but before it in llvm-ar? If this were before the option parsing, you wouldn't need the `HasAixXOption` variable.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1469-1471
+      if (BitMode == BitModeTy::Any || BitMode == BitModeTy::Unknown)
+        fail("The OBJECT_MODE environment variable has an invalid setting. "
+             "OBJECT_MODE must be 32, 64, or 32_64.");
----------------
This is an inconsistency with llvm-ar's current behaviour: llvm-ar treats `Unknown` as 32, whereas here you're outright rejecting it. I'm not convinced this inconsistency makes sense, nor do I see it benefiting the user. In my opinion the two should do the same thing. I think a garbage string should be outright rejected in both cases. An empty string for the environment variable or command-line option should probably be rejected too, but I'm okay with it being accepted, if AIX ranlib behaves that way. However, I think llvm-ranlib and llvm-ar should do the same whatever that is.

Once these inconsistencies have been ironed out, I think it'll be a lot simpler to share code between the two tools.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142660