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

[Pedro Alves <pedro@palves.net>]

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

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.

[-- Attachment #2: align.cpp.gz --]
[-- Type: application/gzip, Size: 15008 bytes --]