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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
