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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
