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

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

[-- Attachment #1: Type: text/plain, Size: 10861 bytes --]

Hi Pedro, Ulrich, Tom and community,

I actually made a mistake while I was experimenting. I copied this line in two places resulting in this hunk by mistake which was not required. That is why I saw the error of initialising a non-static member. I apologise for the same.

 @@ -87,6 +87,7 @@
            puts $outfile "DEF ($utype, $uinner);"
            set joined "${utype}_x_${uinner}"
            puts $outfile "struct align_pair_$joined item_${joined};"
+           puts $outfile "$utype align_pair_${joined}::one = 0;"
            puts $outfile "unsigned a_${joined}"
            puts $outfile "  = ${align_func} (struct align_pair_${joined});"

So AIX numbers are here:-
gdb.base/align-c++ = Passes = 1134
gdb.base/align-c = Passes = 406
gdb.base/align = Passes = 1802

I am okay with the patch with change ID = I874b717afde7b6fb1e45e526912b518a20a12716 pasted to me on the 29th of March. It is the same in AIX as well. Kindly commit the same.


Only one more problem Pedro, Tom and Ulrich..
Consider the code below..

#include <iostream>
using namespace std;
#define DEF(T,U) struct align_pair_ ## T ## _x_ ## U { T one; U two; }
DEF (char, double);
struct align_pair_char_x_double item_char_x_double;
int main (){
    int x = alignof (struct align_pair_char_x_double);
    printf ("%d \n", x);
    return 0;
}

The output of this in linux is 8. But in AIX is 4. Though the size is 8 for double, alignment is 4.

bash-5.1$ ~/gdb_tests/align
4

So we have failures in all test cases involving double data type in AIX.

Using /home/aditya/latest_gdb/binutils-gdb/gdb/testsuite/config/unix.exp as tool-and-target-specific interface file.
Running /home/aditya/latest_gdb/binutils-gdb/gdb/testsuite/gdb.base/align-c.exp ...
FAIL: gdb.base/align-c.exp: print _Alignof(struct align_pair_char_x_double)
FAIL: gdb.base/align-c.exp: print _Alignof(struct align_pair_char_x_long_double)
FAIL: gdb.base/align-c.exp: print _Alignof(struct align_pair_unsigned_char_x_double)
FAIL: gdb.base/align-c.exp: print _Alignof(struct align_pair_unsigned_char_x_long_double)
FAIL: gdb.base/align-c.exp: print _Alignof(struct align_pair_short_x_double)
FAIL: gdb.base/align-c.exp: print _Alignof(struct align_pair_short_x_long_double)
FAIL: gdb.base/align-c.exp: print _Alignof(struct align_pair_unsigned_short_x_double)
FAIL: gdb.base/align-c.exp: print _Alignof(struct align_pair_unsigned_short_x_long_double)
FAIL: gdb.base/align-c.exp: print _Alignof(struct align_pair_int_x_double)
FAIL: gdb.base/align-c.exp: print _Alignof(struct align_pair_int_x_long_double)
FAIL: gdb.base/align-c.exp: print _Alignof(struct align_pair_unsigned_int_x_double)
FAIL: gdb.base/align-c.exp: print _Alignof(struct align_pair_unsigned_int_x_long_double)
FAIL: gdb.base/align-c.exp: print _Alignof(struct align_pair_float_x_double)
FAIL: gdb.base/align-c.exp: print _Alignof(struct align_pair_float_x_long_double)
FAIL: gdb.base/align-c.exp: print _Alignof(double)
FAIL: gdb.base/align-c.exp: print _Alignof(long double)

                === gdb Summary ===

# of expected passes            406
# of unexpected failures        16
/home/aditya/latest_gdb/binutils-gdb/gdb/gdb version  14.0.50.20230327-git -nw -nx -iex "set height 0" -iex "set width 0" -data-directory /home/aditya/latest_gdb/binutils-gdb/gdb/data-directory


Number of failures:-
gdb.base/align-c++ = 42
gdb.base/align-c = 16
gdb.cp/align = 50

Can we discuss this in the same thread or you want me to handle it in a different thread email??Let me know.


Have a nice day ahead.

Thanks and regards,
Aditya.






From: Pedro Alves <pedro@palves.net>
Date: Wednesday, 5 April 2023 at 10:03 PM
To: Aditya Kamath1 <Aditya.Kamath1@ibm.com>, Ulrich Weigand <Ulrich.Weigand@de.ibm.com>, gdb-patches@sourceware.org <gdb-patches@sourceware.org>, tom@tromey.com <tom@tromey.com>
Cc: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>, simon.marchi@efficios.com <simon.marchi@efficios.com>
Subject: [EXTERNAL] Re: [PATCH] Modify align-c/align-c++ test case for AIX
On 2023-03-31 1:29 p.m., Aditya Kamath1 wrote:
> Hi Pedro, Ulrich, Tom and community members,
>
> Thank you for your feedback. Please find attached the patch. {See: 0001-Modify-align-c-align-c-test-case-for-AIX.patch}
>
>>Like below.  Does this work for AIX?
>
> Hi Pedro and Respected community members. Kindly allow me to disagree with you all for the patch diff that was pasted in the previous email reply thread.
>
> Consider a smaller piece of the test case pasted
> #include <iostream>
>
> using namespace std;
>
> struct align_pair_char_x_char {char one; char two;};
>
> struct align_pair_char_x_char item_char_x_char;
>
> *char align_pair_char_x_char::one = 0;*
>
> unsigned a_char_x_char
>
>   = alignof (struct align_pair_char_x_char);
>
> int main() {
>
>   printf ("%d \n", a_char_x_char);
>
> }
>
> So the line highlighted in bold is a problem. This line is also there in the patch diff that was pasted in the previous email reply thread.  This is not allowed. We are trying to access something we should not. In case we want to initialise any data member, we need to do it via a constructor.


I'm super confused.  I don't get a line like the one you highlighted in the generated code at all.

>
> In AIX, I can see 'char align_pair_char_x_unsigned_char::one' is not a static data member of 'struct align_pair_char_x_unsigned_char' error

How did you get that line in the generated code after my patch?

Note the patch has:

> diff --git a/gdb/testsuite/gdb.base/align.exp.tcl b/gdb/testsuite/gdb.base/align.exp.tcl
> index 6a75a14d887..550afe1c47d 100644
> --- a/gdb/testsuite/gdb.base/align.exp.tcl
> +++ b/gdb/testsuite/gdb.base/align.exp.tcl
> @@ -94,12 +94,15 @@ proc prepare_test_source_file { lang } {
>                  puts $outfile "DEF_WITH_1_STATIC ($utype, $uinner);"
>                  set joined "static_${utype}_x_${uinner}"
>                  puts $outfile "struct align_pair_$joined item_${joined};"
> +               puts $outfile "$utype align_pair_${joined}::one = 0;"
>                  puts $outfile "unsigned a_${joined}"
>                  puts $outfile "  = ${align_func} (struct align_pair_${joined});"
>
>                  puts $outfile "DEF_WITH_2_STATIC ($utype, $uinner);"
>                  set joined "static_${utype}_x_static_${uinner}"
>                  puts $outfile "struct align_pair_$joined item_${joined};"
> +               puts $outfile "$utype align_pair_${joined}::one = 0;"
> +               puts $outfile "$uinner align_pair_${joined}::two = 0;"
>                  puts $outfile "unsigned a_${joined}"
>                  puts $outfile "  = ${align_func} (struct align_pair_${joined});"
>              }

DEF_WITH_1_STATIC and DEF_WITH_2_STATIC look like this:

 #define DEF_WITH_1_STATIC(T,U) struct align_pair_static_ ## T ## _x_ ## U { static T one; U two; }
 #define DEF_WITH_2_STATIC(T,U) struct align_pair_static_ ## T ## _x_static_ ## U { static T one; static U two; }

The first has one static field, the second has two static fields.  The lines I added in the
patch hunk above are meant to initialize those static fields.  That's why I have:

Initialize one static field here:

                  puts $outfile "DEF_WITH_1_STATIC ($utype, $uinner);"
                  ...
                  puts $outfile "$utype align_pair_${joined}::one = 0;"

Initialize two static fields here:

                  puts $outfile "DEF_WITH_2_STATIC ($utype, $uinner);"
                  ...
                  puts $outfile "$utype align_pair_${joined}::one = 0;"
                  puts $outfile "$uinner align_pair_${joined}::two = 0;"

Both of these commands came out empty for me, meaning the "::one" and "::two"
initializations are only done on lines with types that have "static" in their name:

 $ grep "::one" testsuite/outputs/gdb.base/align-c++/align.cpp  | grep -v static
 $ grep "::two" testsuite/outputs/gdb.base/align-c++/align.cpp  | grep -v static


Diffing the align.cpp file generated before my patch against a align.cpp file
generated after my patch, I get a lot of changes that looks like this:

--- align.cpp   2023-04-05 17:20:54.002858720 +0100
+++ align-pedro.cpp     2023-04-05 17:20:33.228766715 +0100
@@ -28,10 +28,13 @@ unsigned a_char_x_char
   = alignof (struct align_pair_char_x_char);
 DEF_WITH_1_STATIC (char, char);
 struct align_pair_static_char_x_char item_static_char_x_char;
+char align_pair_static_char_x_char::one = 0;
 unsigned a_static_char_x_char
   = alignof (struct align_pair_static_char_x_char);
 DEF_WITH_2_STATIC (char, char);
 struct align_pair_static_char_x_static_char item_static_char_x_static_char;
+char align_pair_static_char_x_static_char::one = 0;
+char align_pair_static_char_x_static_char::two = 0;
...

Again, the "::one" and "::one" lines only appear on types that have "static"
in their name.

>
>
> I did not try in PowerPc Linux or another Linux machine. But from the little knowledge I have, it should not work there as well. Someone needs to check this in Linux. May be you or Ulrich or Tom can confirm this from your experience in Linux.  Let me know.
>

I had tested my patch on Linux before sending it.  It works here just fine, like so:

 $ make check TESTS="gdb.*/align*.exp"
 Running /home/pedro/rocm/gdb/build-master/gdb/testsuite/../../../src/gdb/testsuite/gdb.base/align-c++.exp ...
 Running /home/pedro/rocm/gdb/build-master/gdb/testsuite/../../../src/gdb/testsuite/gdb.cp/align.exp ...
 Running /home/pedro/rocm/gdb/build-master/gdb/testsuite/../../../src/gdb/testsuite/gdb.base/align-c.exp ...

                 === gdb Summary ===

 # of expected passes            3450


Your patch doesn't fix the issue I pointed at in the commit log, this one:

    For the C++ test, that reveals that the static variable members of the
    generated structs are not defined anywhere, leading to undefined
    references.  Fixed by emitting initialization for all static members.

I.e., if I test with:

 $ make check TESTS="gdb.*/align-*.exp" RUNTESTFLAGS="CC_FOR_TARGET='clang -flto' CXX_FOR_TARGET='clang++ -flto'"

then I see hundreds of failures like these:

 print /d a_static_char_x_char
 Missing ELF symbol "a_static_char_x_char".
 (gdb) FAIL: gdb.base/align-c++.exp: get integer valueof "a_static_char_x_char"
 ...


Going back to this:

> So the line highlighted in bold is a problem. This line is also there in the patch diff that was pasted in the previous email reply thread.  This is not allowed. We are trying to access something we should not. In case we want to initialise any data member, we need to do it via a constructor.

I'm attaching the attach.cpp file that the testsuite generates for me.