[PATCH] D132494: [AIX] llvm-nm support environment "OBJECT_MODE" for option -X on AIX OS

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-nm/option-X-AIX.test:62
+
+## environment variable "OBJECT_MODE" on archive.
+# RUN: llvm-nm --format=just-symbols %t.a | \
----------------
DiggerLin wrote:
> jhenderson wrote:
> > 
> I think we do not need to test the archive here.
I'd recommend we have some sort of archive testing + -X.OBJECT_MODE in llvm-nm tests. Archives are a different enough case internally that even though it may share code ultimately, it's a good idea to check them anyway.

You don't need to test every combination of the environment variable + -X with archives: it would be sufficient (if it doesn't already exist) to show that one or other works with archives when using llvm-nm.


================
Comment at: llvm/test/tools/llvm-nm/option-X-Non-AIX.test:16
+
+## Test the default "-X" option on non-AIX OS.
+# RUN: llvm-nm --format=just-symbols %t_elf32.o %t_elf64.o | \
----------------
No need to repeat this comment twice: probably delete the earlier instance above the YAML.


================
Comment at: llvm/test/tools/llvm-nm/option-X-Non-AIX.test:20
+
+## Test environment variable "OBJECT_MODE" not working on non-AIX OS.
+# RUN: env OBJECT_MODE=32 llvm-nm --format=just-symbols %t_elf32.o %t_elf64.o | \
----------------



================
Comment at: llvm/test/tools/llvm-nm/option-X-Non-AIX.test:1
+# REQUIRES: !system-aix
+
----------------
DiggerLin wrote:
> jhenderson wrote:
> > I've taken another look at this test and am wondering what the test gives us above what regular llvm-nm tests give us?
> the default behavior of llvm-nm is different of AIX os and on Non-AIX os.
> 
> If in Non-AIX OS , if there is no -X option in llvm-nm command the default value of -X is any bits.  (and the Non-AIX also ignore the  OBJECT_MODE environment).
> 
> but for the AIX OS.  if there is no -X option in llvm-nm command the default value of -X is 32 bits(if there is no OBJECT_MODE environment).
> 
> so we need separate test case to test the default value of -X in Non-AIX os.
I think there's definitely value in showing that OBJECT_MODE is ignored on non-AIX, so the second half of this test is useful and should be kept. The first half is not doing anything that other llvm-nm testing won't alreday do: those tests on non-AIX should have coverage for 32 and 64-bit objects and show that they are dumped as expected when requested. We know from the second half of this test that the OBJECT_MODLE=any setting in the test config is ignored, so we don't need to worry about it overriding the default behaviour. Therefore, we can see that the default behaviour is to recognise 32 and 64-bit objects, from those tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132494