DiggerLin marked 3 inline comments as done.
DiggerLin added inline comments.
================
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:
> DiggerLin wrote:
> > 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"))
> >
> So with AIX ar, does an invalid OBJECT_MODE environment variable get reported
> if the -X option is specified?
>
> In my opinion, I think it is very unlikely there will be any real users out
> there with an invalid OBJECT_MODE environment variable, because other tools
> will reject it, even if ranlib doesn't. Even if there are, they should be
> able to easily fix their variable, if they start getting an error message
> after switching to llvm-ranlib. I'm therefore thinking that there isn't a
> need to mirror this logic in llvm-ranlib, if it would make the code simpler
> (which it would).
> So with AIX ar, does an invalid OBJECT_MODE environment variable get reported
> if the -X option is specified?
it do no report invalid OBJECT_MODE environment if the -X option is specified.
```
bash-5.0$ export OBJECT_MODE=31
bash-5.0$ ar q tmpp.acheck.o
ar: 0707-133 The OBJECT_MODE environment variable has an invalid setting.
OBJECT_MODE must be 32, 64, 32_64, d64, or any.
bash-5.0$ ar -X32 q tmpp.acheck.o
bash-5.0$
```
> In my opinion, I think it is very unlikely there will be any real users out
> there with an invalid OBJECT_MODE environment variable, because other tools
> will reject it, even if ranlib doesn't. Even if there are, they should be
> able to easily fix their variable, if they start getting an error message
> after switching to llvm-ranlib. I'm therefore thinking that there isn't a
> need to mirror this logic in llvm-ranlib, if it would make the code simpler
> (which it would).
since -X option will overwrite the env variable OBJECT_MODE,
in the source code , we should check the -X option of the command line first,
if there is no -X option , we will check the env variable OBJECT_MODE. the
logic of code is correct in the patch. we should not always
getenv("OBJECT_MODE") before checking the -X option.
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
