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

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:17-18
+## Test the OBJECT_MODE environment variable when adding symbol table.
+# RUN: unset OBJECT_MODE
+# RUN: llvm-ranlib t_X32.a
+# RUN: llvm-nm --print-armap t_X32.a 2>&1 | FileCheck --check-prefixes=GLOB32 --implicit-check-not="in t64" %s
----------------
Assuming the unsetting is intended to be just for the llvm-ranlib line, I believe the preferred approach is `env -u OBJECT_MODE llvm-ranlib ...`. That way, you don't impact the behaviour in subsequent lines.


================
Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:31
+
+## Test -X option when adding symbol table.
+# RUN: llvm-ranlib -X32 t_X32.a
----------------
jhenderson wrote:
> 
Marked as done but not addressed? Please don't do that...


================
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";
 }
----------------
DiggerLin wrote:
> 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
You didn't actually reflow the string. I believe the following is more correct (I haven't checked the column width, so make sure to run clang-format afterwards:

```
            "should be generated if they do not already exist (AIX OS only)\n";
```


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1442
+
+            // -X option in ranlib do not accept "any"
+            if (BitMode == BitModeTy::Unknown || BitMode == BitModeTy::Any)
----------------
DiggerLin wrote:
> jhenderson wrote:
> > I know that AIX ranlib doesn't accept "any" whereas I believe AIX ar does. I wonder though what the benefit is of preventing llvm-ranlib from accepting "any"? Dropping that special case would simplify the code.
> agree with you. but we discussed about whether to accept `any` in our internal , we decide to keep all the behavior  as AIX `ranlib`
> we decide to keep all the behavior as AIX ranlib

We aren't in your internal company here. This is the open source community, therefore you need to be able to justify your decisions in the open source conversation. What reason is there to keep rejecting this in llvm-ranlib? Perhaps worth asking yourself is "if we could control it, why would we keep that behaviour in AIX ranlib?".


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1463
 
+  if (object::Archive::getDefaultKindForHost() == object::Archive::K_AIXBIG) {
+    // If not specify -X option, get BitMode from enviorment variable
----------------
DiggerLin wrote:
> jhenderson wrote:
> > Is there a particular reason that this is after the command-line option parsing for llvm-ranlib, but before it in llvm-ar? If this were before the option parsing, you wouldn't need the `HasAixXOption` variable.
> in AIX OS  `ranlib` has behavior as
> 
> 
> ```
> -bash-5.0$ env OBJECT_MODE=31 ranlib tmpk.a
> 0654-603 The OBJECT_MODE environment variable has an invalid setting.
>         OBJECT_MODE must be 32, 64, or 32_64.
> -bash-5.0$ env OBJECT_MODE=31 ranlib -X32  tmpk.a
> -bash-5.0$
> ```  
> 
> Given invalid env OBJECT_MODE , if there is no -X option in the ranlib command, it will output as 
> 
> ```
> 0654-603 The OBJECT_MODE environment variable has an invalid setting.
>         OBJECT_MODE must be 32, 64, or 32_64.
> ```
> 
> Given invalid env OBJECT_MODE , and there is -X option in the ranlib command,  it do not care about the invalid env OBJECT_MODE,  So I has to parse the -X option before the getBitMode(getenv("OBJECT_MODE"))
> 
So with AIX ar, does an invalid OBJECT_MODE environment variable get reported if the -X option is specified?

In my opinion, I think it is very unlikely there will be any real users out there with an invalid OBJECT_MODE environment variable, because other tools will reject it, even if ranlib doesn't. Even if there are, they should be able to easily fix their variable, if they start getting an error message after switching to llvm-ranlib. I'm therefore thinking that there isn't a need to mirror this logic in llvm-ranlib, if it would make the code simpler (which it would).


================
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
----------------
DiggerLin wrote:
> 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. 
> 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.

I don't think this is a good reason not to do this, given that it results in an (in my opinion) suboptimal interface. That being said, you could move the new parameter I'm suggesting to the end of the argument list and use a default value, if it helps reduce the impact.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142660