[PATCH] gdb/python/python-internal.h: avoid uninitialized constexpr

[PATCH] gdb/python/python-internal.h: avoid uninitialized constexpr

[Lancelot SIX]

The following recent change introduced a regression when building using
clang++:

    commit 764af878259768bb70c65bdf3f3285c2d6409bbd
    Date:   Wed Jun 12 18:58:49 2024 +0200

        [gdb/python] Add typesafe wrapper around PyObject_CallMethod

The error message is:

    ../../gdb/python/python-internal.h:151:16: error: default initialization of an object of const type 'const char'
    constexpr char gdbpy_method_format;
                   ^
                                       = '\0'
      CXX    python/py-block.o
    1 error generated.
    make[2]: *** [Makefile:1959: python/py-arch.o] Error 1
    make[2]: *** Waiting for unfinished jobs....
    In file included from ../../gdb/python/py-auto-load.c:25:
    ../../gdb/python/python-internal.h:151:16: error: default initialization of an object of const type 'const char'
    constexpr char gdbpy_method_format;
                   ^
                                       = '\0'
    1 error generated.
    make[2]: *** [Makefile:1959: python/py-auto-load.o] Error 1
    In file included from ../../gdb/python/py-block.c:23:
    ../../gdb/python/python-internal.h:151:16: error: default initialization of an object of const type 'const char'
    constexpr char gdbpy_method_format;
                   ^
                                       = '\0'
    1 error generated.

This patch fixes this by changing gdbpy_method_format to be a templated
struct, and only have its specializations define the static constexpr
member "format".  This way, we avoid having an uninitialized constexpr
expression, regardless of it being instantiated or not.

Change-Id: I5bec241144f13500ef78daea30f00d01e373692d
---
 gdb/python/python-internal.h | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index f4c35babe46..fec0010a444 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -148,19 +148,31 @@ typedef long Py_hash_t;
 /* A template variable holding the format character (as for
    Py_BuildValue) for a given type.  */
 template<typename T>
-constexpr char gdbpy_method_format;
+struct gdbpy_method_format {};
 
 template<>
-constexpr char gdbpy_method_format<gdb_py_longest> = GDB_PY_LL_ARG[0];
+struct gdbpy_method_format<gdb_py_longest>
+{
+  static constexpr char format = GDB_PY_LL_ARG[0];
+};
 
 template<>
-constexpr char gdbpy_method_format<gdb_py_ulongest> = GDB_PY_LLU_ARG[0];
+struct gdbpy_method_format<gdb_py_ulongest>
+{
+  static constexpr char format = GDB_PY_LLU_ARG[0];
+};
 
 template<>
-constexpr char gdbpy_method_format<int> = 'i';
+struct gdbpy_method_format<int>
+{
+  static constexpr char format = 'i';
+};
 
 template<>
-constexpr char gdbpy_method_format<unsigned> = 'I';
+struct gdbpy_method_format<unsigned>
+{
+  static constexpr char format = 'I';
+};
 
 /* A helper function to compute the PyObject_CallMethod /
    Py_BuildValue format given the argument types.  */
@@ -169,7 +181,7 @@ template<typename... Args>
 constexpr std::array<char, sizeof... (Args) + 1>
 gdbpy_make_fmt ()
 {
-  return { gdbpy_method_format<Args>..., '\0' };
+  return { gdbpy_method_format<Args>::format..., '\0' };
 }
 
 /* Typesafe wrapper around PyObject_CallMethod.

base-commit: 0915235d341841ac7f13bd3136991c19b4a6746b
-- 
2.34.1

Re: [PATCH] gdb/python/python-internal.h: avoid uninitialized constexpr

[Tom de Vries]

On 6/18/24 11:09, Lancelot SIX wrote:
> The following recent change introduced a regression when building using
> clang++:
> 
>      commit 764af878259768bb70c65bdf3f3285c2d6409bbd
>      Date:   Wed Jun 12 18:58:49 2024 +0200
> 
>          [gdb/python] Add typesafe wrapper around PyObject_CallMethod
> 
> The error message is:
> 
>      ../../gdb/python/python-internal.h:151:16: error: default initialization of an object of const type 'const char'
>      constexpr char gdbpy_method_format;
>                     ^
>                                         = '\0'
>        CXX    python/py-block.o
>      1 error generated.
>      make[2]: *** [Makefile:1959: python/py-arch.o] Error 1
>      make[2]: *** Waiting for unfinished jobs....
>      In file included from ../../gdb/python/py-auto-load.c:25:
>      ../../gdb/python/python-internal.h:151:16: error: default initialization of an object of const type 'const char'
>      constexpr char gdbpy_method_format;
>                     ^
>                                         = '\0'
>      1 error generated.
>      make[2]: *** [Makefile:1959: python/py-auto-load.o] Error 1
>      In file included from ../../gdb/python/py-block.c:23:
>      ../../gdb/python/python-internal.h:151:16: error: default initialization of an object of const type 'const char'
>      constexpr char gdbpy_method_format;
>                     ^
>                                         = '\0'
>      1 error generated.
> 
> This patch fixes this by changing gdbpy_method_format to be a templated
> struct, and only have its specializations define the static constexpr
> member "format".  This way, we avoid having an uninitialized constexpr
> expression, regardless of it being instantiated or not.
> 

Hi Lancelot,

thanks for looking into this.

This seems to have been my doing, since I dropped the " = '\0'" bit.

I can reproduce the problem.

I also tried adding back the dropped bit, and saw that it fixes the 
compilation error.

However, that also brings back the problem I was trying to solve: doing 
this:
...
-      gdbpy_ref<> result = gdbpy_call_method (m_window, "close");
+      gdbpy_ref<> result = gdbpy_call_method (m_window, "close", 1.0);
...
compiles, using '\0' as format specificier for double, and we only run 
into trouble a runtime.

The patch you propose both:
- fixes the build with clang, and
- gives a compile time error for the unsupported format specifier case.

So, LGTM.

Reviewed-By: Tom de Vries <tdevries@suse.de>

Thanks,
- Tom

> Change-Id: I5bec241144f13500ef78daea30f00d01e373692d
> ---
>   gdb/python/python-internal.h | 24 ++++++++++++++++++------
>   1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> index f4c35babe46..fec0010a444 100644
> --- a/gdb/python/python-internal.h
> +++ b/gdb/python/python-internal.h
> @@ -148,19 +148,31 @@ typedef long Py_hash_t;
>   /* A template variable holding the format character (as for
>      Py_BuildValue) for a given type.  */
>   template<typename T>
> -constexpr char gdbpy_method_format;
> +struct gdbpy_method_format {};
>   
>   template<>
> -constexpr char gdbpy_method_format<gdb_py_longest> = GDB_PY_LL_ARG[0];
> +struct gdbpy_method_format<gdb_py_longest>
> +{
> +  static constexpr char format = GDB_PY_LL_ARG[0];
> +};
>   
>   template<>
> -constexpr char gdbpy_method_format<gdb_py_ulongest> = GDB_PY_LLU_ARG[0];
> +struct gdbpy_method_format<gdb_py_ulongest>
> +{
> +  static constexpr char format = GDB_PY_LLU_ARG[0];
> +};
>   
>   template<>
> -constexpr char gdbpy_method_format<int> = 'i';
> +struct gdbpy_method_format<int>
> +{
> +  static constexpr char format = 'i';
> +};
>   
>   template<>
> -constexpr char gdbpy_method_format<unsigned> = 'I';
> +struct gdbpy_method_format<unsigned>
> +{
> +  static constexpr char format = 'I';
> +};
>   
>   /* A helper function to compute the PyObject_CallMethod /
>      Py_BuildValue format given the argument types.  */
> @@ -169,7 +181,7 @@ template<typename... Args>
>   constexpr std::array<char, sizeof... (Args) + 1>
>   gdbpy_make_fmt ()
>   {
> -  return { gdbpy_method_format<Args>..., '\0' };
> +  return { gdbpy_method_format<Args>::format..., '\0' };
>   }
>   
>   /* Typesafe wrapper around PyObject_CallMethod.
> 
> base-commit: 0915235d341841ac7f13bd3136991c19b4a6746b

Re: [PATCH] gdb/python/python-internal.h: avoid uninitialized constexpr

[Tom de Vries]

On 6/18/24 15:00, Tom de Vries wrote:
> On 6/18/24 11:09, Lancelot SIX wrote:
>> The following recent change introduced a regression when building using
>> clang++:
>>
>>      commit 764af878259768bb70c65bdf3f3285c2d6409bbd
>>      Date:   Wed Jun 12 18:58:49 2024 +0200
>>
>>          [gdb/python] Add typesafe wrapper around PyObject_CallMethod
>>
>> The error message is:
>>
>>      ../../gdb/python/python-internal.h:151:16: error: default 
>> initialization of an object of const type 'const char'
>>      constexpr char gdbpy_method_format;
>>                     ^
>>                                         = '\0'
>>        CXX    python/py-block.o
>>      1 error generated.
>>      make[2]: *** [Makefile:1959: python/py-arch.o] Error 1
>>      make[2]: *** Waiting for unfinished jobs....
>>      In file included from ../../gdb/python/py-auto-load.c:25:
>>      ../../gdb/python/python-internal.h:151:16: error: default 
>> initialization of an object of const type 'const char'
>>      constexpr char gdbpy_method_format;
>>                     ^
>>                                         = '\0'
>>      1 error generated.
>>      make[2]: *** [Makefile:1959: python/py-auto-load.o] Error 1
>>      In file included from ../../gdb/python/py-block.c:23:
>>      ../../gdb/python/python-internal.h:151:16: error: default 
>> initialization of an object of const type 'const char'
>>      constexpr char gdbpy_method_format;
>>                     ^
>>                                         = '\0'
>>      1 error generated.
>>
>> This patch fixes this by changing gdbpy_method_format to be a templated
>> struct, and only have its specializations define the static constexpr
>> member "format".  This way, we avoid having an uninitialized constexpr
>> expression, regardless of it being instantiated or not.
>>
> 
> Hi Lancelot,
> 
> thanks for looking into this.
> 
> This seems to have been my doing, since I dropped the " = '\0'" bit.
> 
> I can reproduce the problem.
> 
> I also tried adding back the dropped bit, and saw that it fixes the 
> compilation error.
> 
> However, that also brings back the problem I was trying to solve: doing 
> this:
> ...
> -      gdbpy_ref<> result = gdbpy_call_method (m_window, "close");
> +      gdbpy_ref<> result = gdbpy_call_method (m_window, "close", 1.0);
> ...
> compiles, using '\0' as format specificier for double, and we only run 
> into trouble a runtime.
> 
> The patch you propose both:
> - fixes the build with clang, and
> - gives a compile time error for the unsupported format specifier case.
> 
> So, LGTM.
> 
> Reviewed-By: Tom de Vries <tdevries@suse.de>
> 

FWIW, I also did a build-with-clang and test run, and only see the usual 
suspects + clang-build specific PR31237.

Thanks,
- Tom

>> Change-Id: I5bec241144f13500ef78daea30f00d01e373692d
>> ---
>>   gdb/python/python-internal.h | 24 ++++++++++++++++++------
>>   1 file changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
>> index f4c35babe46..fec0010a444 100644
>> --- a/gdb/python/python-internal.h
>> +++ b/gdb/python/python-internal.h
>> @@ -148,19 +148,31 @@ typedef long Py_hash_t;
>>   /* A template variable holding the format character (as for
>>      Py_BuildValue) for a given type.  */
>>   template<typename T>
>> -constexpr char gdbpy_method_format;
>> +struct gdbpy_method_format {};
>>   template<>
>> -constexpr char gdbpy_method_format<gdb_py_longest> = GDB_PY_LL_ARG[0];
>> +struct gdbpy_method_format<gdb_py_longest>
>> +{
>> +  static constexpr char format = GDB_PY_LL_ARG[0];
>> +};
>>   template<>
>> -constexpr char gdbpy_method_format<gdb_py_ulongest> = GDB_PY_LLU_ARG[0];
>> +struct gdbpy_method_format<gdb_py_ulongest>
>> +{
>> +  static constexpr char format = GDB_PY_LLU_ARG[0];
>> +};
>>   template<>
>> -constexpr char gdbpy_method_format<int> = 'i';
>> +struct gdbpy_method_format<int>
>> +{
>> +  static constexpr char format = 'i';
>> +};
>>   template<>
>> -constexpr char gdbpy_method_format<unsigned> = 'I';
>> +struct gdbpy_method_format<unsigned>
>> +{
>> +  static constexpr char format = 'I';
>> +};
>>   /* A helper function to compute the PyObject_CallMethod /
>>      Py_BuildValue format given the argument types.  */
>> @@ -169,7 +181,7 @@ template<typename... Args>
>>   constexpr std::array<char, sizeof... (Args) + 1>
>>   gdbpy_make_fmt ()
>>   {
>> -  return { gdbpy_method_format<Args>..., '\0' };
>> +  return { gdbpy_method_format<Args>::format..., '\0' };
>>   }
>>   /* Typesafe wrapper around PyObject_CallMethod.
>>
>> base-commit: 0915235d341841ac7f13bd3136991c19b4a6746b
> 

Re: [PATCH] gdb/python/python-internal.h: avoid uninitialized constexpr

[Tom Tromey]

>>>>> "Lancelot" == Lancelot SIX <lancelot.six@amd.com> writes:

Lancelot>  template<typename T>
Lancelot> -constexpr char gdbpy_method_format;
Lancelot> +struct gdbpy_method_format {};
 
Would it work to just remove the 'constexpr' from this line?

Tom

Re: [PATCH] gdb/python/python-internal.h: avoid uninitialized constexpr

[Lancelot SIX]

On 18/06/2024 17:17, Tom Tromey wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
>>>>>> "Lancelot" == Lancelot SIX <lancelot.six@amd.com> writes:
> 
> Lancelot>  template<typename T>
> Lancelot> -constexpr char gdbpy_method_format;
> Lancelot> +struct gdbpy_method_format {};
> 
> Would it work to just remove the 'constexpr' from this line?
> 
> Tom

Just gave a try to this approach, I end up linker errors regarding 
duplicated symbols:

ld.lld: error: duplicate symbol: gdbpy_method_format<long long>
 >>> defined at python-internal.h:154 
(../../gdb/python/python-internal.h:154)
 >>>            python/py-arch.o:(gdbpy_method_format<long long>)
 >>> defined at python-internal.h:154 
(../../gdb/python/python-internal.h:154)
 >>>            python/py-auto-load.o:(.data+0x0)

ld.lld: error: duplicate symbol: gdbpy_method_format<unsigned long long>
 >>> defined at python-internal.h:157 
(../../gdb/python/python-internal.h:157)
 >>>            python/py-arch.o:(gdbpy_method_format<unsigned long long>)
 >>> defined at python-internal.h:157 
(../../gdb/python/python-internal.h:157)
 >>>            python/py-auto-load.o:(.data+0x20)

I have not really looked if there is a way to work-around that.

Best,
Lancelot.

Re: [PATCH] gdb/python/python-internal.h: avoid uninitialized constexpr

[Tom Tromey]

Lancelot> Just gave a try to this approach, I end up linker errors regarding
Lancelot> duplicated symbols:

Lancelot> I have not really looked if there is a way to work-around that.

I think s/constexpr/extern probably works.
Would you mind trying that?

Tom

Re: [PATCH] gdb/python/python-internal.h: avoid uninitialized constexpr

[Lancelot SIX]

On 18/06/2024 20:48, Tom Tromey wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> Lancelot> Just gave a try to this approach, I end up linker errors regarding
> Lancelot> duplicated symbols:
> 
> Lancelot> I have not really looked if there is a way to work-around that.
> 
> I think s/constexpr/extern probably works.
> Would you mind trying that?
> 
> Tom

G++ complains that "explicit template specialization cannot have a 
storage class":

In file included from ../../gdb/python/py-arch.c:23:
../../gdb/python/python-internal.h:154:13: error: ‘gdbpy_method_format’ 
initialized and declared ‘extern’ [-Werror]
   154 | extern char gdbpy_method_format<gdb_py_longest> = GDB_PY_LL_ARG[0];
       |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../gdb/python/python-internal.h:154:1: error: explicit template 
specialization cannot have a storage class
   154 | extern char gdbpy_method_format<gdb_py_longest> = GDB_PY_LL_ARG[0];
       | ^~~~~~

Clang++ complains because "'extern' variable has an initializer".  I 
know the empty struct as a base template, and add static constexpr 
members for template specializations is a bit verbose, but to the best 
of my knowledge it is the "c++ way".

Lancelot.

Re: [PATCH] gdb/python/python-internal.h: avoid uninitialized constexpr

[Tom Tromey]

>>>>> "Lancelot" == Lancelot SIX <Lancelot.Six@amd.com> writes:

>> I think s/constexpr/extern probably works.

Lancelot> G++ complains that "explicit template specialization cannot have a
Lancelot> storage class":

I meant just the constexpr on the "base case", not:

Lancelot>   154 | extern char gdbpy_method_format<gdb_py_longest> = GDB_PY_LL_ARG[0];

... this one -- these have to be constexpr.

Tom

Re: [PATCH] gdb/python/python-internal.h: avoid uninitialized constexpr

[Lancelot SIX]

On 18/06/2024 22:03, Tom Tromey wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
>>>>>> "Lancelot" == Lancelot SIX <Lancelot.Six@amd.com> writes:
> 
>>> I think s/constexpr/extern probably works.
> 
> Lancelot> G++ complains that "explicit template specialization cannot have a
> Lancelot> storage class":
> 
> I meant just the constexpr on the "base case", not:
> 
> Lancelot>   154 | extern char gdbpy_method_format<gdb_py_longest> = GDB_PY_LL_ARG[0];
> 
> ... this one -- these have to be constexpr.
> 
> Tom

Oh, sorry, that makes more sense.

That works with GCC, but still fails with Clang.  I still have 
duplicate symbol errors (both at -O0 and -O3).  My understanding is that 
the compiler can still handle constexr as const if it wants, so marking 
those symbols constexpr will not necessarily prevent them from being 
materialized in the binary.

Lancelot.

Re: [PATCH] gdb/python/python-internal.h: avoid uninitialized constexpr

[Tom Tromey]

>>>>> "Lancelot" == Lancelot SIX <Lancelot.Six@amd.com> writes:

Lancelot> That works with GCC, but still fails with Clang.  I still have
Lancelot> duplicate symbol errors (both at -O0 and -O3).  My understanding is
Lancelot> that the compiler can still handle constexr as const if it wants, so
Lancelot> marking those symbols constexpr will not necessarily prevent them from
Lancelot> being materialized in the binary.

Which symbol is duplicate?

Anyway if that is the case, then why does your patch work?

Tom

Re: [PATCH] gdb/python/python-internal.h: avoid uninitialized constexpr

[Lancelot SIX]

On 19/06/2024 01:11, Tom Tromey wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
>>>>>> "Lancelot" == Lancelot SIX <Lancelot.Six@amd.com> writes:
> 
> Lancelot> That works with GCC, but still fails with Clang.  I still have
> Lancelot> duplicate symbol errors (both at -O0 and -O3).  My understanding is
> Lancelot> that the compiler can still handle constexr as const if it wants, so
> Lancelot> marking those symbols constexpr will not necessarily prevent them from
> Lancelot> being materialized in the binary.
> 
> Which symbol is duplicate?

All the specialized instantiations appear as duplicates.

With your solution, if I compile with GCC, I have:

$ nm -C gdb/python/py-arch.o|grep gdbpy_method_format
0000000000000380 r gdbpy_method_format<int>
00000000000003c0 r gdbpy_method_format<unsigned int>
0000000000000300 r gdbpy_method_format<long long>
0000000000000340 r gdbpy_method_format<unsigned long long>

while when compiling with clang:

$ nm -C gdb/python/py-arch.o|grep gdbpy_method_format
0000000000000040 R gdbpy_method_format<int>
0000000000000060 R gdbpy_method_format<unsigned int>
0000000000000000 R gdbpy_method_format<long long>
0000000000000020 R gdbpy_method_format<unsigned long long>

It looks like clang picks the "extern" qualifier from the 
non-specialized case, and applies it to all the specializations.

It seems I can avoid this by marking the specializations as inline, i.e.

template<typename T>
extern char gdbpy_method_format;

template<>
constexpr inline char gdbpy_method_format<gdb_py_longest> = 
GDB_PY_LL_ARG[0];

...

This approach seems to work for both clang and gcc.  I can modify my 
patch to use this method if you prefer.

> 
> Anyway if that is the case, then why does your patch work?
> 

In my approach, it seems the symbol is not be present in the object 
files for neither gcc or clang.  If I force it (by taking the address of 
the symbol for example), nm reports the symbol as a weak object ("V") 
for clang and "u" for gcc.

Best,
Lancelot.

Re: [PATCH] gdb/python/python-internal.h: avoid uninitialized constexpr

[Tom Tromey]

>>>>> "Lancelot" == Lancelot SIX <Lancelot.Six@amd.com> writes:

Lancelot> template<>
Lancelot> constexpr inline char gdbpy_method_format<gdb_py_longest> =
Lancelot> GDB_PY_LL_ARG[0];

Lancelot> This approach seems to work for both clang and gcc.  I can modify my
Lancelot> patch to use this method if you prefer.

>> Anyway if that is the case, then why does your patch work?

Lancelot> In my approach, it seems the symbol is not be present in the object
Lancelot> files for neither gcc or clang.

Ok, I read here:

https://en.cppreference.com/w/cpp/language/inline

that:

    A static data member declared constexpr on its first declaration is
    implicitly an inline variable.

... which explains why wrapping it in a class works.

I'm satisfied now that there's an explanation.  I personally think the
inline constexpr variable is more aesthetically pleasing but you can
push your original patch if you prefer.

Tom

Re: [PATCH] gdb/python/python-internal.h: avoid uninitialized constexpr

[Lancelot SIX]

> 
> I'm satisfied now that there's an explanation.  I personally think the
> inline constexpr variable is more aesthetically pleasing but you can
> push your original patch if you prefer.
> 
> Tom

Thanks,

I have pushed this patch as ea4e03c0a9f5ed8574dcaa434cd979a6f92f5c1f.

Best,
Lancelot.