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

Digger Lin via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 16 10:57:39 PDT 2022


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


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



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


================
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)) ||
----------------
jhenderson wrote:
> This would be a more normal way of calling it.
thanks


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


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:707-708
+        // Check whether the object mode is permit to output.
+        if (!IsValidInBitMode(C))
+          continue;
+
----------------
jhenderson wrote:
> 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.
good catch, thanks


================
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,
----------------
jhenderson wrote:
> 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.
if we check the object BEFORE, the file will be opened  checking for object mode,  and we need to open the file somewhere as NewArchiveMember . otherwise we have to open the "FileName" again. 

if we open the "FileName" and keep as NewArchiveMember somewhere, the problem is that if the FileName is archive , it can not be kept in the NewArchiveMember.




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


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1285
+    if (BitMode == BitModeTy::Unknown)
+      BitMode = BitModeTy::Bit32;
+  }
----------------
jhenderson wrote:
> 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).
if there is no OBJECT_MODE environment variable set and no -X option in AIX "ar" , The default is to process 32-bit object files (ignore 64-bit objects).  
https://www.ibm.com/docs/ko/aix/7.1?topic=ar-command


================
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);
----------------
jhenderson wrote:
> What's the motivation for making this AIX OS only?
I think to make -X for all the non AIX OS before, but in the AIX,  The default is to process 32-bit object files (ignore 64-bit objects).  but  for other non AIX OS, the default is to process both 32 and 64 bits object file. it maybe caused problem for the script which llvm-ar.

if you think it is better to let -X work for non AIX OS, I can make 
default for AIX is 32bit, for non AIX OS, the default is any.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127864



More information about the llvm-commits mailing list