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

[Digger Lin via Phabricator via cfe-commits]

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 Deterministic = true;         ///< 'D' and 'U' modifiers
----------------
jhenderson wrote:
> Maybe I'm missing something, but I don't see why you need to make this a custom type. You already have the BitMode value that you read in from the environment/command-line, and so you can just use that in conjunction with the `Symtab` boolean to determine what symbol table to write, surely?
the Symtab are use to specify whether the symbol table need to write(for non-AIX). and what the symbol table need to write for AIX OS.

there are function like

```
  writeArchiveToBuffer(ArrayRef<NewArchiveMember> NewMembers,
                     WriteSymTabType WriteSymtab, object::Archive::Kind Kind,
                     bool Deterministic, bool Thin)
```
and 

```
Error writeArchive(StringRef ArcName, ArrayRef<NewArchiveMember> NewMembers,
                   WriteSymTabType WriteSymtab, object::Archive::Kind Kind,
                   bool Deterministic, bool Thin,
                   std::unique_ptr<MemoryBuffer> OldArchiveBuf = nullptr);
```

 call the function as 


```
        writeArchiveToBuffer(M.second.getMembers(),
                             /*WriteSymtab=*/true,
                             /*Kind=*/object::Archive::K_DARWIN,
                             C.Deterministic,
                             /*Thin=*/false); 
```

I do not want to change the calling parameter of the /*WriteSymtab=*/true, 

I introduced a new class WriteSymTabType which has following API
    
 
```
 WriteSymTabType(bool PrintSym) { Value = PrintSym ? Both : None; }
  void operator=(bool PrintSym) { Value = PrintSym ? Both : None; }
  operator bool() { return Value != None; }
```





================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1090-1091
+
+    // If BigArchive, if there is 32-bit globol symbol table, we still use -X64
+    // to generate the 64-bit global symbol table. Same as -X32 option.
+    if (OldArchive->kind() == object::Archive::K_AIXBIG) {
----------------
jhenderson wrote:
> Just to make sure I'm following what this is trying to say: for Big Archives, a -X64 specification will ignore the 32-bit symbol table and generate a 64-bit one (the 32-bit one will remain in place, if present), and using -X32 will cause a 32-bit symbol table to be generated, right?
> 
> Assuming that's the case, I suggest the following as the comment:
> 
> "For archives in the Big Archive format, the bit mode option specifies which symbol table to generate. The presence of a symbol table that does not match the specified bit mode does not prevent creation of the symbol table that has been requested."
yes, you are correct. I change the comment to your suggest.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1421
+  // specified.
+  if (object::Archive::getDefaultKindForHost() == object::Archive::K_AIXBIG) {
+    BitMode = getBitMode(getenv("OBJECT_MODE"));
----------------
jhenderson wrote:
> I think you can share this code with the llvm-ar version of the code (put it in a function). You can either add an "AcceptAny" member to that function's argument, or simply check if the value is "Any" after the function here.
In AIX OS ,since  ranlib do not support -Xany. it is different to share code with llvm-ar 

-bash-5.0$ ranlib -Xany
0654-602 The specified object mode is not valid.
        Specify -X32, -X64, or -X32_64.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1423
+    BitMode = getBitMode(getenv("OBJECT_MODE"));
+    // -X option in ranlib do not accept "any"
+    if (BitMode == BitModeTy::Unknown || BitMode == BitModeTy::Any)
----------------
jhenderson wrote:
> Does it treat "any" as "32" or does it report an error? If the latter, should we not do the same here?
-Xany , I will report an error.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1446
+        } else if (arg.front() == 'X') {
+          if (object::Archive::getDefaultKindForHost() ==
+              object::Archive::K_AIXBIG) {
----------------
jhenderson wrote:
> Similar to above, could we refactor things slightly to share the code for parsing the -X option between llvm-ranlib and llvm-ar?
the logic of parsing option is different with ar_main, it is difficult to share the code. and the llvm-ranlib do not support -Xany,  and there only a few lines of duplication code, I do not think it worth us to put effort to share the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142660