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