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

[James Henderson via Phabricator via cfe-commits]

jhenderson added a comment.

In D142660#4541406 <https://reviews.llvm.org/D142660#4541406>, @jhenderson wrote:

> In D142660#4538936 <https://reviews.llvm.org/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, then, if the -X option is present, overwrite that variable, and finally feed that string into the parsing code that converts into a BitMode value. If the string is invalid, the parsing code could report an error along the lines of "invalid OBJECT_MODE or -X option value".
>>
>> if I do not think it is better than my current implement, If I implement as your suggestion, I need another variable to recoded the where the string come from(from OBJECT_MODE  or -X option value). otherwise when the invalid value of string , how can I report it is an invalid OBJECT_MODE"  or "invalid -X option value"
>
> I'm not convinced you need to distinguish the two. If a user hasn't specified the -X option, then they'll know it's the OBJECT_MODE environment variable. Even if they have both, it should be obvious to a quick glance of the full error message what the wrong value is (because you'd include it in the message) and you could then it won't take long to find which of the two is wrong (even if they don't know that -X trumps OBJET_MODE). That being said, I still prefer the parse OBJECT_MODE first, report an error, then later read the -X option and check for an error there.

It looks like you've ignored this point in your most recent changes. I agree that having a variable to say whether the BitMode variable comes from OBJECT_MODE or -X is not great, and I'd prefer the `HasAIXOption` style you had before over it. However, I'd still rather the invalid OBJECT_MODE value be read and rejected upfront before even parsing the -X option.



================
Comment at: llvm/include/llvm/Object/ArchiveWriter.h:44
+enum SymtabWritingMode {
+  NoSymtab,     // Not write Symbol table.
+  NormalSymtab, // Write both 32-bit and 64-bit symbol table.
----------------



================
Comment at: llvm/include/llvm/Object/ArchiveWriter.h:45
+  NoSymtab,     // Not write Symbol table.
+  NormalSymtab, // Write both 32-bit and 64-bit symbol table.
+  Bit32Only,    // Only write the 32-bit symbol table.
----------------
This comment doesn't make sense for non-Big Archive archives, because regular archives only have one symbol table. There is no concept of a 32- or 64-bit one. Perhaps "Write the symbol table. For the Big Archive format, writes both 32-bit and 64-bit symbol tables."


================
Comment at: llvm/include/llvm/Object/ArchiveWriter.h:46-47
+  NormalSymtab, // Write both 32-bit and 64-bit symbol table.
+  Bit32Only,    // Only write the 32-bit symbol table.
+  Bit64Only     // Only write the 64-bit symbol table.
+};
----------------
I'd prefix these two with `BigArchive`, or even just name them `BigArchive32` and `BigArchive64` respectively, to more clearly convey the fact that they are specific to that file format.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:966
   if (!isAIXBigArchive(Kind)) {
-    if (WriteSymtab) {
+    if (WriteSymtab != SymtabWritingMode::NoSymtab) {
       if (!HeadersSize)
----------------
Where this comparison is repeated more than once in the same function, it might be nice to store the value in a local boolean variable for use in all places instead of repeating the comparison.


================
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.
----------------
Looks like there's a typo in your test name?


================
Comment at: llvm/test/tools/llvm-ranlib/non-AIX-not-supportedwq-X-option.test:6
+
+# INVALID-X-OPTION: error: -X32 option not supported on non AIX OS 
----------------
Nit: trailing whitespace


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142660