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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin marked 10 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1394
           object::Archive::K_AIXBIG) {
         Match = *(*ArgIt + 2) != '\0' ? *ArgIt + 2 : *(++ArgIt);
         BitMode = getBitMode(Match);
----------------
jhenderson wrote:
> 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`?
```
-bash-5.0$ /home/zhijian/llvm/dev/build/bin/llvm-ar -X  tmpk.a d_default.o

/home/zhijian/llvm/dev/build/bin/llvm-ar: error: invalid bit mode: tmpk.a


-bash-5.0$ ar -X tmpk.a d_default.o
ar: 0707-132 The specified object mode is not valid.
        Specify -X32, -X64, -X32_64, -Xd64, or -Xany

-bash-5.0$llvm-ranlib -X  tmpk.a d_default.o
llvm-ranlib: error: the specified object mode is not valid. Specify -X32, -X64, or -X32_64

-bash-5.0$ ranlib -X  tmpk.a d_default.o
0654-602 The specified object mode is not valid.
        Specify -X32, -X64, or -X32_64.
```

I will change the behaviors of llvm-ar in a new separate patch. 


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1437
+            const char *Xarg = arg.data();
+            if (Xarg[0] == '\0')
+              BitMode = BitModeTy::Unknown;
----------------
jhenderson wrote:
> 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)?
thanks. add functionality to accept -X 32


================
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)
----------------
jhenderson wrote:
> 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.
agree with you. but we discussed about whether to accept `any` in our internal , we decide to keep all the behavior  as AIX `ranlib`


================
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
----------------
jhenderson wrote:
> 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.
in AIX OS  `ranlib` has behavior as


```
-bash-5.0$ env OBJECT_MODE=31 ranlib tmpk.a
0654-603 The OBJECT_MODE environment variable has an invalid setting.
        OBJECT_MODE must be 32, 64, or 32_64.
-bash-5.0$ env OBJECT_MODE=31 ranlib -X32  tmpk.a
-bash-5.0$
```  

Given invalid env OBJECT_MODE , if there is no -X option in the ranlib command, it will output as 

```
0654-603 The OBJECT_MODE environment variable has an invalid setting.
        OBJECT_MODE must be 32, 64, or 32_64.
```

Given invalid env OBJECT_MODE , and there is -X option in the ranlib command,  it do not care about the invalid env OBJECT_MODE,  So I has to parse the -X option before the getBitMode(getenv("OBJECT_MODE"))



================
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.");
----------------
jhenderson wrote:
> 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.
In AIX OS , `nm` and `ar` , do not accept invalid env var OBJECT_MODE, for example, I will create two separate patches to fix the behavior of llvm-nm and llvm-ar (they look invalid env var OBJECT_MODE as default 32bit)


```
bash-5.0$ env OBJECT_MODE=31 nm   d_default.o
0654-216 nm: The OBJECT_MODE environment variable has an invalid setting.
        OBJECT_MODE must be 32, 64, 32_64, d64 or any.
-bash-5.0$ env OBJECT_MODE=31 ar  -q tmpk.a d_default.o
ar: 0707-133 The OBJECT_MODE environment variable has an invalid setting.
        OBJECT_MODE must be 32, 64, 32_64, d64, or any. 
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142660