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

This revision was automatically updated to reflect the committed changes. Closed by commit rGf740bcb3707a: [AIX] supporting -X options for llvm-ranlib in AIX OS (authored by zhijian zhij...@ca.ibm.com). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

DiggerLin added a comment. @MaskRay, would you have any additional comments to share? If not, would it be appropriate for me to proceed with committing the patch? Your input would be greatly appreciated. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

DiggerLin updated this revision to Diff 550776. DiggerLin marked 3 inline comments as done. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142660/new/ https://reviews.llvm.org/D142660 Files: clang/lib/Driver/OffloadBundler.cpp

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

MaskRay added inline comments. Comment at: llvm/include/llvm/Object/ArchiveWriter.h:43 +enum SymtabWritingMode { + NoSymtab, // Do not write symbol table. Below you use `SymtabWritingMode::` for all members, so just make this enum scoped.

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

MaskRay added a comment. `# REQUIRES: system-aix` in llvm/test/tools/llvm-ranlib/aix-X-option.test unfortunately makes most -X tests only work on AIX, which most contributors don't have access to. Ideally `-X` is usable when the user specifies `--format=bigarchive`. The environment variable

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

jhenderson accepted this revision. jhenderson added a comment. I'm going to accept this change, although I still have significant concerns about how the whole parsing logic seems more complicated than it needs to be. Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:74 + <<

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

DiggerLin added inline comments. Comment at: clang/test/lit.cfg.py:391 if "system-aix" in config.available_features: -config.substitutions.append(("llvm-nm", "env OBJECT_MODE=any llvm-nm")) -config.substitutions.append(("llvm-ar", "env OBJECT_MODE=any llvm-ar")) +

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

DiggerLin updated this revision to Diff 549938. DiggerLin marked 4 inline comments as done. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142660/new/ https://reviews.llvm.org/D142660 Files: clang/lib/Driver/OffloadBundler.cpp

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

jhenderson added a comment. clang-format is complaining in the pre-merge CI. Comment at: clang/test/lit.cfg.py:391 if "system-aix" in config.available_features: -config.substitutions.append(("llvm-nm", "env OBJECT_MODE=any llvm-nm")) -

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

DiggerLin updated this revision to Diff 546867. DiggerLin marked 2 inline comments as done. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142660/new/ https://reviews.llvm.org/D142660 Files: clang/lib/Driver/OffloadBundler.cpp

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

MaskRay added inline comments. Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:74 + << " -U- Use actual timestamps and uids/gids\n" + << " -X{32|64|32_64|any} - Specifies which archive symbol tables " + "should be generated if they do not

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

DiggerLin updated this revision to Diff 546444. DiggerLin marked an inline comment as done. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142660/new/ https://reviews.llvm.org/D142660 Files: clang/lib/Driver/OffloadBundler.cpp

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

MaskRay added a comment. I glanced at the patch. The code seems reasonable. Comment at: clang/test/lit.cfg.py:383 -# The llvm-nm tool supports an environment variable "OBJECT_MODE" on AIX OS, which +# Some tool support an environment variable "OBJECT_MODE" on AIX OS, which

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

DiggerLin marked an inline comment as done. DiggerLin added inline comments. Comment at: llvm/test/tools/llvm-ranlib/non-AIX-not-supportedwq-X-option.test:1 +## REQUIRES: !system-aix +## Test the -X option is not supported on non-AIX os. jhenderson wrote: >

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

DiggerLin updated this revision to Diff 546071. DiggerLin marked 5 inline comments as done. DiggerLin added a comment. address comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142660/new/ https://reviews.llvm.org/D142660 Files:

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

DiggerLin added a comment. > I'd still rather the invalid OBJECT_MODE value be read and rejected upfront > before even parsing the -X option. as Stephen and me point out, if there is -X option, we do not care about the environment env `OBJECT-MODE` , when user input the -X option , it means ,

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

jhenderson added a comment. In D142660#4541406 , @jhenderson wrote: > In D142660#4538936 , @DiggerLin > wrote: > >>> As an alternative (but I think adds unnecessary complexity, due to needing >>> an extra

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

DiggerLin updated this revision to Diff 545392. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142660/new/ https://reviews.llvm.org/D142660 Files: clang/lib/Driver/OffloadBundler.cpp clang/test/lit.cfg.py

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

DiggerLin updated this revision to Diff 545247. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142660/new/ https://reviews.llvm.org/D142660 Files: clang/lib/Driver/OffloadBundler.cpp clang/test/lit.cfg.py

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

jhenderson added a comment. In D142660#4538936 , @DiggerLin wrote: >> As an alternative (but I think adds unnecessary complexity, due to needing >> an extra variable), you could have both tools read the environment variable >> into a string variable,

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

DiggerLin updated this revision to Diff 544804. DiggerLin added a comment. addressed comment and let llvm-ranlib supports -Xany option Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142660/new/ https://reviews.llvm.org/D142660 Files:

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

DiggerLin added a comment. > I do see the appeal of a single enum parameter in the meantime. I would > suggest it should be called SymtabWritingMode and have values NoSymtab, > NormalSymtab, BigArchive64Bit, BigArchive32Bit, and BigArchiveBoth. You could > optionally drop one of the

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

DiggerLin marked 2 inline comments as done. DiggerLin added a comment. > As an alternative (but I think adds unnecessary complexity, due to needing an > extra variable), you could have both tools read the environment variable into > a string variable, then, if the -X option is present,

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

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:

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

MaskRay added inline comments. Comment at: clang/test/lit.cfg.py:391 +if 'system-aix' in config.available_features: + config.environment['OBJECT_MODE'] = '32_64' Recent Python style prefers double quotes Repository: rG LLVM Github Monorepo CHANGES SINCE

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

jhenderson added a comment. In D142660#4535693 , @stephenpeckham wrote: > I don't see any reason to check the OBJECT_MODE environment variable if the > -X flag is used. What would the error be: "You specified a valid -X flag, > but by the way,

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

stephenpeckham accepted this revision. stephenpeckham added a comment. This revision is now accepted and ready to land. I don't see any reason to check the OBJECT_MODE environment variable if the -X flag is used. What would the error be: "You specified a valid -X flag, but by the way,

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

jhenderson added inline comments. Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:68 + +## Test the invalid -X option and OBJECT_MODE enviornment var. +# RUN: not env OBJECT_MODE= llvm-ranlib t_X32.a 2>&1 | FileCheck --implicit-check-not="error:"

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

DiggerLin updated this revision to Diff 544066. DiggerLin marked an inline comment as done. DiggerLin added a comment. addressed partial comment, after deciding whether to delete the `bool NeedSymbols parameter` from functions API,(writeArchiveToStream etc), I will upload a update patch.

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

DiggerLin marked 2 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:

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

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

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

DiggerLin marked 2 inline comments as done. DiggerLin added inline comments. 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)

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

DiggerLin marked an inline comment as done. DiggerLin added inline comments. Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:237 static bool Verbose = false; ///< 'v' modifier -static bool Symtab = true;///< 's' modifier +static WriteSymTabType Symtab =

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

jhenderson 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

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

DiggerLin updated this revision to Diff 542660. DiggerLin added a comment. 1. rebase the code 2. for the `env OBJECT_MODE=` or is `env OBJECT_MODE=""` , the `ranlib` and `ar` in AIX OS , have different behaviors(ranlib looks as -X32 , but `ar` and `nm` output error ), I change `llvm-ranlib` as

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

DiggerLin updated this revision to Diff 542209. DiggerLin marked 5 inline comments as done. DiggerLin added a comment. address comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142660/new/ https://reviews.llvm.org/D142660 Files:

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

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);

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

DiggerLin marked 14 inline comments as done. DiggerLin added a comment. In D142660#4509464 , @jhenderson wrote: > I still have concerns about the option-parsing logic being duplicated, but > I'm out of time to review it now. I'll try to look tomorrow.

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

jhenderson added inline comments. Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:3 +## Test the -X option. +## The option specifies the type of object file on which llvm-ranlib will operate. + Nit: trailing whitespace Comment at:

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

jhenderson added a comment. My apologies, this patch fell off my review queue somehow. I still have concerns about the option-parsing logic being duplicated, but I'm out of time to review it now. I'll try to look tomorrow. Comment at:

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

DiggerLin added a comment. Gentle ping , @jhenderson 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

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

DiggerLin updated this revision to Diff 523759. DiggerLin added a comment. rebase to latest master after the patch https://reviews.llvm.org/D142479 [AIX] support 64bit global symbol table for big archive landed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

jhenderson added a comment. I haven't looked again at the rest of this patch. I'll do so hopefully in the next couple of weeks. Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:80 + << " -U- Use actual timestamps and uids/gids\n" + << " -X

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

DiggerLin added inline comments. Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:16 + +## Test OBJECT_MODE environment variable when adding symbol table +# RUN: env OBJECT_MODE=32 llvm-ranlib t_X32.a stephenpeckham wrote: > what about OBJECT_MODE=

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

DiggerLin updated this revision to Diff 522625. DiggerLin marked 4 inline comments as done. DiggerLin added a reviewer: stephenpeckham. DiggerLin added a comment. address Stephen's comment, thanks Stephen. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

stephenpeckham added a comment. Do the -U and -D flags have any effect on the behavior of llvm-ranlib? Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:16 + +## Test OBJECT_MODE environment variable when adding symbol table +# RUN: env OBJECT_MODE=32 llvm-ranlib

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

DiggerLin added inline comments. Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:237 static bool Verbose = false; ///< 'v' modifier -static bool Symtab = true;///< 's' modifier +static WriteSymTabType Symtab = true; ///< 's' modifier static bool

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

DiggerLin updated this revision to Diff 517638. DiggerLin marked 9 inline comments as done. DiggerLin added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. address comment and set option -Xany as invalid option and environment OBJECT-MODE=any as invalid too(let