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

[Digger Lin via Phabricator via cfe-commits]

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


================
Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:17-18
+## Test the OBJECT_MODE environment variable when adding symbol table.
+# RUN: unset OBJECT_MODE
+# RUN: llvm-ranlib t_X32.a
+# RUN: llvm-nm --print-armap t_X32.a 2>&1 | FileCheck --check-prefixes=GLOB32 --implicit-check-not="in t64" %s
----------------
jhenderson wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > Assuming the unsetting is intended to be just for the llvm-ranlib line, I believe the preferred approach is `env -u OBJECT_MODE llvm-ranlib ...`. That way, you don't impact the behaviour in subsequent lines.
> > the command `env` of AIX OS do not support -u.  and there is no mapping  option of `env -u`
> > 
> > https://www.ibm.com/docs/en/aix/7.2?topic=e-env-command
> I'm 90% certain that `env` here is the built-in lit `env`, not a system `env` executable. `env -u` seems to be only rarely used in the test suite, but see llvm/utils/lit/tests/Inputs/shtest-env/env-u.txt for an example. I assume you can run this test locally?
it run fail in AIX OS locally.


================
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:
> > > 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.
> > 
> > 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.
> 
> I don't agree. It is normal to validate all of a a user's input options (including environment variables that are a proxy for command-line options) even if they don't end up getting used. Consider a similar, admittedly somewhat arbitrary case. If you had one command called e.g. `--doTheThing` whose sole purpose was to ensure that some `if` block was run, and then another option `--doTheThingThisWay={methodA|methodB}` which controlled what happens within that `if` block, it would be perfectly normal and acceptable to check the value passed to `--doTheThingThisWay` even without `--doTheThing` being specified. The code would typically look like this pseudo code:
> ```
> Config processCommandLine(Args args) {
>   Config cfg;
>   if ("doTheThing" in args) {
>     cfg.doTheThing = true;
>   }
>   if ("doTheThingThisWay" in args) {
>     switch(args["doTheThingThisWay"].value) {
>        case "methodA":
>         cfg.doTheThingThisWay = DoTheThingThisWayKind::MethodA;
>         break;
>        case "methodB":
>         cfg.doTheThingThisWay = DoTheThingThisWayKind::MethodB;
>         break;
>       default:
>         reportError("invalid --doTheThingThisWay value");
>     }
>   }
> }
> ```
> The OBJECT_MODE environment variable is a proxy for the -X option. Therefore, it should be parsed and checked in the same manner as the -X option even if it isn't used. This brings consistency between the two tools, which is good. Incosistency is bad: it increases code complexity and potentially can confuse users. What is the benefit to making the two tools inconsistent?
> The OBJECT_MODE environment variable is a proxy for the -X option. Therefore, it should be parsed and checked in the same manner as the -X option even if it isn't used. This brings consistency between the two tools, which is good. Incosistency is bad: it increases code complexity and potentially can confuse users. What is the benefit to making the two tools inconsistent?

as I mention before , I will create  patches to let the llvm-ar and llvm-nm to work same as logic  llvm-ranlib for the env variable "OBJECT_MODE" 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142660