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

[Digger Lin via Phabricator via cfe-commits]

DiggerLin marked 14 inline comments as done.
DiggerLin added a comment.

In D142660#4509464 <https://reviews.llvm.org/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.

In AIX OS , `nm` and `ar` , do not accept invalid env var OBJECT_MODE, for example, I will create two patch to fix the behavior of llvm-nm and llvm-ar (they look invalid env var OBJECT_MODE as default 32bit)

  bash-5.0$ env OBJECT_MODE=31 nm   d_default.o
  0654-216 nm: The OBJECT_MODE environment variable has an invalid setting.
          OBJECT_MODE must be 32, 64, 32_64, d64 or any.
  -bash-5.0$ env OBJECT_MODE=31 ar  -q tmpk.a d_default.o
  ar: 0707-133 The OBJECT_MODE environment variable has an invalid setting.
          OBJECT_MODE must be 32, 64, 32_64, d64, or any. 





================
Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:17
+## Test OBJECT_MODE environment variable when adding symbol table
+# RUN: env OBJECT_MODE= llvm-ranlib t_X32.a
+# RUN: env OBJECT_MODE=32 llvm-ranlib t_X32.a
----------------
jhenderson wrote:
> Doesn't this line want an llvm-nm check after it, like the others?
yes, thanks


================
Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:67
+
+#GL64-NOT:    var_0x1F7 in t64_1.o
+#GL64-NOT:    array_0x1F7 in t64_1.o
----------------
jhenderson wrote:
> I'm concerned you're going to have an ordering issue in one of the 64 bit or 32 bit "NOT" cases. FileCheck -NOT patterns only check the region between the previous non-NOT pattern before the -NOT one up to the next non-NOT pattern (or the end of the file). So, as things stand, --check-prefixes=GLOB32,GL64 will verify that the 32-bit table appears and then no 64-bit table is printed AFTER the 32-bit table, but not whether the 64-bit table appears BEFORE the 32-bit table. Similarly, --check-prefixes=GLOB64,GL32 will check that the 64-bit table is printed and then no 32-bit table is printed after it, but not whether the 32-bit table is printed before it.
> 
> Given that the names of the symbols are irrelevant to this test case, and really all that's interesting is which object they're in (since this test isn't about checking that the symbol tables have the correct contents - that is the duty of a test that doesn't make use of the -X option), could you simplify the input objects to have a single like-named symbol and then simply ensure the correct object names appear in the output? You could use --implicit-check-not to check that e.g. "in t64" or "in t32" don't appear in your output.
> 
> If you adopted this approach, you'd have two CHECK lines as e.g. follows:
> ```
> # GLOB32:     symbol1 in t32_1.o
> # GLOB32-NEXT: symbol2 in t32_2.o
> 
> # GLOB64:     symbol1 in t64_1.o
> # GLOB64-NEXT: symbol2 in t64_2.o
> ```
> And you'd have FileCheck lines that look like one of the following:
> ```
> FileCheck --check-prefixes=GLOB32 --implicit-check-not="in t64"
> FileCheck --check-prefixes=GLOB64 --implicit-check-not="in t32"
> FileCheck --check-prefixes=GLOB32,GLOB64
> ```
> 
> By simplifying down to one symbol each, you could also pass the symbol name in as a yaml2obj -D value and only have one YAML file in the test.
thanks for detail explain, I changed as your suggestion.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:74-75
+         << "  -X {32|64|32_64}      - Specifies which archive symbol tables "
+            "should"
+            "be generated if they do not already exist (AIX OS only)\n";
 }
----------------
jhenderson wrote:
> Please reflow this string. That should make it fairly obvious that there's a mistake in it too (hint: print the help and spot the missing space...).
thanks


================
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:
> DiggerLin wrote:
> > 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; }
> > ```
> > 
> > 
> > 
> Having looked at this again, I really don't like the implicit conversion semantics of the new class, and I don't see why you can't just extend the `writeArchiveToBuffer` etc parameters to pass in a `BitMode` parameter.
without implicit conversion, if I add a new parameter BitMode to writeArchiveToBuffer,  I also need to add the new parameter BitMode to function writeArchive() and writeArchiveToStream() and there is a lot of place(files) call the function writeArchive, I need to modify all the caller too. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142660