Re: [PATCH] Modify align-c/align-c++ test case for AIX

[Aditya Kamath1 <Aditya.Kamath1@ibm.com>]

[-- Attachment #1.1: Type: text/plain, Size: 8552 bytes --]

Hi Ulrich, Tom, Pedro and community members,

Appreciate your patience and feedback. I had to check a few things before we go ahead. Sorry for the delay in response.

>I see the -Wl,-bnogc option there, this makes sense.

>I'm not sure why "-lodm -lcfg" should be necessary as well, this
>doesn't really appear related to me.  Can you try without those?

So here is the problem.

bash-5.1$ gcc -g -gdwarf ~/gdb_tests/simple_test.c -o ~/gdb_tests/simple_test -Wl,-bnogc
ld: 0711-317 ERROR: Undefined symbol: .odm_initialize
ld: 0711-317 ERROR: Undefined symbol: CuDv_CLASS
ld: 0711-317 ERROR: Undefined symbol: .odm_get_list
ld: 0711-317 ERROR: Undefined symbol: .odm_free_list
ld: 0711-317 ERROR: Undefined symbol: .getattr
ld: 0711-317 ERROR: Undefined symbol: .odm_terminate
ld: 0711-345 Use the -bloadmap or -bnoquiet option to obtain more information.
collect2: error: ld returned 8 exit status

When we say no garbage collection, the linker shows these errors. These are garbage symbols. Nowhere in my code pasted below I have used any odm modules symbol. But since the linker is not collecting garbage, this seems to be having symbols that needs to link with ODM and that is not a desired behaviour. So I was using -lodm, -lcfg. But this should not be the case. Another problem I have seen is when we try to link with other modules like -lm {Math} or -lpthread {pthread}, we see similar issues.

The code :-

bash-5.1$ cat ~/gdb_tests/simple_test.c
#include <stdio.h>
int __attribute__((unused)) global_variable1 = 1;
int global_variable2 = 2;
int main(){
        int local_variable = 1;
        printf ("Simple print statement \n");
        return 0;
}

Since this is an inconsistent behaviour while using the -Wl,-bnogc I have decided not to use the same. We do not want the linker to allow any garbage symbols just because we gave an option to it to not collect garbage.

Instead in the documentation { https://www.ibm.com/docs/en/aix/7.1?topic=l-ld-command } I have seen one more option which is more specific.

For the same code above say we compile using the command
bash-5.1$ gcc -g -gdwarf ~/gdb_tests/simple_test.c -o ~/gdb_tests/simple_test -Wl,-uglobal_variable1
bash-5.1$ ./gdb ~/gdb_tests/simple_test
GNU gdb (GDB) 14.0.50.20230327-git
Copyright (C) 2023 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later http://gnu.org/licenses/gpl.html
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Type "show copying" and "show warranty" for details.
This GDB was configured as "powerpc64-ibm-aix7.2.0.0".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
https://www.gnu.org/software/gdb/bugs/.
Find the GDB manual and other documentation resources online at:
    http://www.gnu.org/software/gdb/documentation/.

For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from /home/aditya/gdb_tests/simple_test...
(gdb) p global_variable1
$1 = 1
(gdb) p global_variable2
$2 = 2
(gdb)

So this -u option “-uSymbolName” tells the linker to only allow variable with name SymbolName or any variable used in this code only. This can be a much better option for us in this testcase than -Wl,bnogc which is collecting any random symbol which garbage. Please find attached a patch {See: 0001-Modify-align-c-align-c-test-case-for-AIX_v2.patch}. I have Tested this and it worked.

Test case passes after patch :- align-c = 406 align-c++ = 1134
Test case passes before patch:- align-c = 0  align-c++ = 0

Partial Dwarf dump output for the same code above

<1><  357>      DW_TAG_variable
                DW_AT_name                  global_variable1
                DW_AT_decl_file             2
                DW_AT_decl_line             2
                DW_AT_decl_column           29
                DW_AT_type                  <185>
                DW_AT_external              yes
                DW_AT_location              DW_OP_addr 0x20000ea0
<1><  388>      DW_TAG_variable
                DW_AT_name                  global_variable2
                DW_AT_decl_file             2
                DW_AT_decl_line             3
                DW_AT_decl_column           5
                DW_AT_type                  <185>
                DW_AT_external              yes
                DW_AT_location              DW_OP_addr 0x20000ea4

This is one option..

>I'm assuming this is only needed with test files that use global
>variables unused in the source code, but used by the debugger.
>I think it would be best to explicitly add the flag to those tests
>where it is needed.

Yes. Your assumption is absolutely correct Ulrich. If unused and no garbage collection option given then in my previous mails dwarf dump output we can see linker adds 0xfffffff as the address of garbage variables.

>I'm wondering whether __attribute__((used)) on the global variable makes any
>difference.  IIRC with LLVM it prevents link-time stripping on Mach-O and PE/COFF,
>but I don't know about XCOFF + GCC.  (It doesn't on ELF, you
>need 'retain' for that.)

Hi Pedro. So thanks for this suggestion. I learnt something new from you. But this __attribute__((used)) did not make any difference in our environment.

>With a comment
>somewhere explaining why, I'm also ok with that.  It seems less
>intrusive maybe than adding ldflags to a lot of tests.

>Well, that was Aditya's original proposal, I guess ...   I'd be fine
>with that as well - whatever is easier.

Hi Ulrich and Tom. Yes, so the second option we have is to do this. I have attached my original patch proposal as well. Please find attached. {See: 0001-Modify-align-c-align-c-test-case-for-AIX_v1.patch}.

So whatever you feel is right from the community angle we should go with that. But yeah, if we use {See: 0001-Modify-align-c-align-c-test-case-for-AIX_v2.patch} we know why we are doing so and test case will look more like our Linux folks.

>I think doing a global run with -Wl,-bnogc would still be interesting,
>simply to get a list of all the tests that would need changing.

So I can tell you without these options what is going on.
Currently in the gdb.base test suite we pass 87%  { 27139  passes and 3918 FAILS } tests in AIX, with the major failures in
align-c -• 412 failures
align-c++ • 1146 failures
whatis-ptype • 1062 failures
attach • 40 failures..

In gdb.cp as well we have 96% tests passed in AIX, with major failure in
align.exp and next-overthrow.exp • 50 failures each..

All these failures are expected to reduce with this fix. Have given numbers with the patch for align-c and align-c++. Not sure of attach and whatis-pytpe. I don’t think that has to do with this patch. Will see what is going on in those two test cases and come up with patches.

Kindly push one of these patches if there are no changes. If not kindly suggest me a better way you think we can do this together.

Have a nice day ahead.

Thanks and regards,
Aditya.







From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Date: Tuesday, 21 March 2023 at 7:56 PM
To: gdb-patches@sourceware.org <gdb-patches@sourceware.org>, tom@tromey.com <tom@tromey.com>
Cc: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>, Aditya Kamath1 <Aditya.Kamath1@ibm.com>, simon.marchi@efficios.com <simon.marchi@efficios.com>
Subject: Re: [PATCH] Modify align-c/align-c++ test case for AIX
Tom Tromey <tom@tromey.com> wrote:
>Ulrich> If the -bnogc flag is always necessary with the AIX linker, this
>Ulrich> should be handled automatically by the test instead of via site.exp.
>
>Ulrich> I'm assuming this is only needed with test files that use global
>Ulrich> variables unused in the source code, but used by the debugger.
>Ulrich> I think it would be best to explicitly add the flag to those tests
>Ulrich> where it is needed.
>
>I think there's also cases in the test suite where we've added uses of
>variables to ensure they aren't optimized away.  With a comment
>somewhere explaining why, I'm also ok with that.  It seems less
>intrusive maybe than adding ldflags to a lot of tests.

Well, that was Aditya's original proposal, I guess ...   I'd be fine
with that as well - whatever is easier.

I think doing a global run with -Wl,-bnogc would still be interesting,
simply to get a list of all the tests that would need changing.

Bye,
Ulrich

[-- Attachment #2: 0001-Modify-align-c-align-c-test-case-for-AIX_v1.patch --]
[-- Type: application/octet-stream, Size: 1004 bytes --]

From 5daed42ecd34d0019f3a2e4828a5b014ee865452 Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Fri, 10 Mar 2023 01:22:14 -0600
Subject: [PATCH] Modify align-c/align-c++ test case for AIX

On AIX, the debugger cannot access global variables before they
they are used in any function. Hence we add this small change so
that we can test the same in AIX.
---
 gdb/testsuite/gdb.base/align.exp.tcl | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/gdb/testsuite/gdb.base/align.exp.tcl b/gdb/testsuite/gdb.base/align.exp.tcl
index 6a75a14d887..f18243080ce 100644
--- a/gdb/testsuite/gdb.base/align.exp.tcl
+++ b/gdb/testsuite/gdb.base/align.exp.tcl
@@ -109,6 +109,11 @@ proc prepare_test_source_file { lang } {
     # Epilogue.
     puts $outfile {
 	int main() {
+	    #ifdef _AIX
+	    /* On AIX, the debugger cannot access global variables before they
+		they are used in the functions.  */
+		a_char_x_char++;
+	    #endif
 	    return 0;
 	}
     }
-- 
2.38.3


[-- Attachment #3: 0001-Modify-align-c-align-c-test-case-for-AIX_v2.patch --]
[-- Type: application/octet-stream, Size: 1189 bytes --]

From 27aedd08296fe880bc2f2c74f9ff68713ce14245 Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Wed, 29 Mar 2023 05:18:45 -0500
Subject: [PATCH] Modify align-c/align-c++ test case for AIX

On AIX, the debugger cannot access global variables before they
they are used in any function. Hence we add this small change so
that we can test the same in AIX.
---
 gdb/testsuite/gdb.base/align.exp.tcl | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/gdb/testsuite/gdb.base/align.exp.tcl b/gdb/testsuite/gdb.base/align.exp.tcl
index 6a75a14d887..18a370df72d 100644
--- a/gdb/testsuite/gdb.base/align.exp.tcl
+++ b/gdb/testsuite/gdb.base/align.exp.tcl
@@ -129,6 +129,11 @@ proc run_alignment_test { lang } {
     if { "$lang" == "c++" } {
 	lappend flags "additional_flags=-std=c++11"
     }
+    if { [istarget *-*-aix*] } {
+        # Disable linker garbage collection to avoid optimizing
+        # out global variables unused in the test source code.
+        lappend flags "ldflags=-Wl,-ua_char_x_char"
+    }
     standard_testfile $filename
     if {[prepare_for_testing "failed to prepare" "$testfile" $srcfile $flags]} {
 	return -1
-- 
2.38.3