[PATCH] D127864: [llvm-ar] Add object mode option -X for AIX

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

The pre-merge checks are showing up failures caused by this patch. Please fix them.

I've not looked at the tests yet.



================
Comment at: llvm/docs/CommandGuide/llvm-ar.rst:294
+
+ Specifies the type of object file ar should examine. The mode must be one of the following:
+
----------------
I don't like the word "examine" here. llvm-ar is usually about manipulating archives, as much as (if not more than) inspecting them. How about "will recognise" instead of "should examine"?


================
Comment at: llvm/docs/CommandGuide/llvm-ar.rst:303
+   any
+         Process all the supported object files.
+
----------------
I don't think "supported" is really necessary, so let's keep the message simple.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:620
 
+static bool isSymbolList64Bit(SymbolicFile &Obj) {
+  if (auto *IRObj = dyn_cast<IRObjectFile>(&Obj))
----------------
I don't know what you mean by "SymbolList" here and why that has anything to do with bitness.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:636
+
+static bool IsValidInBitMode(Binary &Bin) {
+  if (BitMode == BitModeTy::Bit32_64 || BitMode == BitModeTy::Any)
----------------
Please fix this function name to conform to LLVM coding standards.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:641
+  if (SymbolicFile *SymFile = dyn_cast<SymbolicFile>(&Bin)) {
+    bool IsBit64 = isSymbolList64Bit(*SymFile);
+    if ((IsBit64 && (BitMode == BitModeTy::Bit32)) ||
----------------
This would be a more normal way of calling it.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:646-647
+  }
+  // In AIX "ar", If there is member which is not a object file, it also always
+  // be consider as valid bit mode.
+  return true;
----------------



================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:651
+
+static bool IsValidInBitMode(const Archive::Child &C) {
+
----------------
Please fix this function name to conform to LLVM coding standards.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:652
+static bool IsValidInBitMode(const Archive::Child &C) {
+
+  if (object::Archive::getDefaultKindForHost() != object::Archive::K_AIXBIG)
----------------
Delete the extra blank line.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:657-658
+  Expected<std::unique_ptr<Binary>> ChildOrErr = C.getAsBinary();
+  // In AIX "ar", If there is member which is not a object file, it also always
+  // be consider as valid bit mode.
+  if (!ChildOrErr) {
----------------
Same comment change request as above.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:673
+
+static bool IsValidInBitMode(const NewArchiveMember &NM) {
+  if (object::Archive::getDefaultKindForHost() != object::Archive::K_AIXBIG)
----------------
Please fix this function name to conform to LLVM coding standards.

This function looks pretty much identical to the one above. Please refactor the code to avoid duplication.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:707-708
+        // Check whether the object mode is permit to output.
+        if (!IsValidInBitMode(C))
+          continue;
+
----------------
Does this check need to be inside `if (Filter)`? It seems like it's unnecessary and you can avoid duplicating the check in the `else if` by moving it out.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:781-783
+// If the Members is valid object mode, push back into 'Members' and return
+// true; otherwise do not push back into 'Members' and return false.
+static bool addMember(std::vector<NewArchiveMember> &Members,
----------------
I don't like this AT ALL. It would seem FAR better if you check the type of the object BEFORE actually doing any work with it. That way, you don't have to undo any modifications that you might have made.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1223
 
+static BitModeTy GetBitMode(const char *RawBitMode) {
+  return StringSwitch<BitModeTy>(RawBitMode)
----------------
Please make this function name conform to LLVM style.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1281
 
+  // Get BitMode from enviorment variable "OBJECT_MODE" for AIX OS if there is.
+  if (object::Archive::getDefaultKindForHost() == object::Archive::K_AIXBIG) {
----------------



================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1283
+  if (object::Archive::getDefaultKindForHost() == object::Archive::K_AIXBIG) {
+    BitMode = GetBitMode(getenv("OBJECT_MODE"));
+    if (BitMode == BitModeTy::Unknown)
----------------
Is "OBJECT_MODE" an environment variable from AIX ar or is it specific to llvm-ar? If the latter, why do you need it?


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1285
+    if (BitMode == BitModeTy::Unknown)
+      BitMode = BitModeTy::Bit32;
+  }
----------------
Is this the same default value as on AIX? I'd expect "any" to make much more sense as a default (but we should do what AIX ar does).


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1338-1339
+    if (strncmp(*ArgIt, "-X", 2) == 0) {
+      if (object::Archive::getDefaultKindForHost() ==
+          object::Archive::K_AIXBIG) {
+        Match = *(*ArgIt + 2) != '\0' ? *ArgIt + 2 : *(++ArgIt);
----------------
What's the motivation for making this AIX OS only?


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1343
+        if (BitMode == BitModeTy::Unknown)
+          fail(std::string("Invalid Bit Mode: ") + Match);
+        continue;
----------------
This message does not conform to the LLVM coding standard for error messages. Please fix it.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1346
+      } else {
+        fail(Twine(*ArgIt) + " option not supported in no AIX OS.");
+      }
----------------
This message does not conform to the LLVM coding standard for error messages. Please fix it.

Additionally "in no AIX OS" -> "on non-AIX OS"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127864