[PATCH] Fix AIX build break

[PATCH] Fix AIX build break

[Aditya Kamath1]

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

Respected community members,

Hi,

Our CI in AIX for GDB has been broken for the last two days with the following error.

CXXLD  gdb
ld: 0711-317 ERROR: Undefined symbol: _ZTH23deprecated_warning_hook
ld: 0711-317 ERROR: Undefined symbol: ._ZTH23deprecated_warning_hook
ld: 0711-345 Use the -bloadmap or -bnoquiet option to obtain more information.
collect2: error: ld returned 8 exit status
gmake: *** [Makefile:2184: gdb] Error 1

These symbols came due to the commit https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=54b815ddb428944a70694e3767a0fadbdd9ca9ea

The problem in AIX is marking a variable for thread storage as thread_local that makes that symbol a weak symbol. The dump output of the complaints.o shows this as pasted here:-
# dump -tov -X64 complaints.o | grep _ZTH23deprecated_warning_hook
[32]    m   0x00000000     undef     1    weak                    _ZTH23deprecated_warning_hook
[36]    m   0x00000000     undef     1    weak                    ._ZTH23deprecated_warning_hook
[3209]  m   0x000203d8     .data     1  unamex                    _ZTH23deprecated_warning_hook

Hence in the final stage of compilation while creating the binary gdb, the ld error came and the compilation is unsuccessful since the symbol is not visible though complaints.c file was successfully compiled.

This patch is a fix for the same where instead of thread_local we used __thread and GDB now compiles in AIX. Please find attached the patch. (See: 0001-Fix-AIX-build-break.patch).

I want to know your opinion about this. Also, I do not know the impact of this change on other targets. If we can do this better, kindly let me know.

Have a nice day ahead.

Thanks and regards,
Aditya.

[-- Attachment #2: 0001-Fix-AIX-build-break.patch --]
[-- Type: application/octet-stream, Size: 2632 bytes --]

From 9fd8e08fea93e631d86cfca2856d5d8dc0da4da9 Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Fri, 12 Jan 2024 01:30:58 -0600
Subject: [PATCH] Fix AIX build break.

A recent commit broke AIX build. The thread_local type defined functions
were being considered a weak symbol and hence while creating the binary these
symbols were not visible. If we use __thread instead then this issue is fixed in AIX.

This patch is a fix for the same.
---
 gdb/complaints.c | 2 +-
 gdb/complaints.h | 2 +-
 gdb/defs.h       | 2 +-
 gdb/top.c        | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/complaints.c b/gdb/complaints.c
index eb648c655ed..306e4ed4bc8 100644
--- a/gdb/complaints.c
+++ b/gdb/complaints.c
@@ -79,7 +79,7 @@ clear_complaints ()
 
 /* See complaints.h.  */
 
-thread_local complaint_interceptor *complaint_interceptor::g_complaint_interceptor;
+__thread complaint_interceptor *complaint_interceptor::g_complaint_interceptor;
 
 /* See complaints.h.  */
 
diff --git a/gdb/complaints.h b/gdb/complaints.h
index 1626f200685..4dc1b7b70e4 100644
--- a/gdb/complaints.h
+++ b/gdb/complaints.h
@@ -103,7 +103,7 @@ class complaint_interceptor
     ATTRIBUTE_PRINTF (1, 0);
 
   /* This object.  Used by the static callback function.  */
-  static thread_local complaint_interceptor *g_complaint_interceptor;
+  static __thread complaint_interceptor *g_complaint_interceptor;
 };
 
 /* Re-emit complaints that were collected by complaint_interceptor.
diff --git a/gdb/defs.h b/gdb/defs.h
index 2f771d8dc49..089e1c1a909 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -562,7 +562,7 @@ extern void (*deprecated_print_frame_info_listing_hook) (struct symtab * s,
 							 int noerror);
 extern int (*deprecated_query_hook) (const char *, va_list)
      ATTRIBUTE_FPTR_PRINTF(1,0);
-extern thread_local void (*deprecated_warning_hook) (const char *, va_list)
+extern __thread void (*deprecated_warning_hook) (const char *, va_list)
      ATTRIBUTE_FPTR_PRINTF(1,0);
 extern void (*deprecated_readline_begin_hook) (const char *, ...)
      ATTRIBUTE_FPTR_PRINTF_1;
diff --git a/gdb/top.c b/gdb/top.c
index 009bf2b0c1c..20d8eb6029a 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -221,7 +221,7 @@ int (*deprecated_query_hook) (const char *, va_list);
 
 /* Replaces most of warning.  */
 
-thread_local void (*deprecated_warning_hook) (const char *, va_list);
+__thread void (*deprecated_warning_hook) (const char *, va_list);
 
 /* These three functions support getting lines of text from the user.
    They are used in sequence.  First deprecated_readline_begin_hook is
-- 
2.41.0

Re: [PATCH] Fix AIX build break

[Tom de Vries]

On 1/12/24 10:20, Aditya Kamath1 wrote:
> Respected community members,
> 
> Hi,
> 
> Our CI in AIX for GDB has been broken for the last two days with the 
> following error.
> 
> CXXLD  gdb
> 
> ld: 0711-317 ERROR: Undefined symbol: _ZTH23deprecated_warning_hook
> 
> ld: 0711-317 ERROR: Undefined symbol: ._ZTH23deprecated_warning_hook
> 
> ld: 0711-345 Use the -bloadmap or -bnoquiet option to obtain more 
> information.
> 
> collect2: error: ld returned 8 exit status
> 
> gmake: *** [Makefile:2184: gdb] Error 1
> 
> These symbols came due to the commit 
> https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=54b815ddb428944a70694e3767a0fadbdd9ca9ea <https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=54b815ddb428944a70694e3767a0fadbdd9ca9ea>
> 

I think this is triggered by using thread-local with extern.

I found this commit:
...
commit fece451c2aca57b095e7e4063e342781cf74aa75
Author: Christian Biesinger <cbiesinger@google.com>
Date:   Tue Mar 9 08:16:23 2021 -0600

     Use RAII to set the per-thread SIGSEGV handler

     This avoids using a thread-local extern variable, which causes link 
errors
     on some platforms, notably Cygwin.  But I think this is a better 
pattern
     even outside of working around linker bugs because it encapsulates 
direct
     access to the variable inside the class, instead of having a global 
extern
     variable.
...
which seems related, given that it mentions link errors.  Perhaps we can 
do something similar here.

Thanks,
- Tom

> The problem in AIX is marking a variable for thread storage as 
> thread_local that makes that symbol a weak symbol. The dump output of 
> the complaints.o shows this as pasted here:-
> 
> # dump -tov -X64 complaints.o | grep _ZTH23deprecated_warning_hook
> 
> [32]    m   0x00000000     undef     1    weak                    
> _ZTH23deprecated_warning_hook
> 
> [36]    m   0x00000000     undef     1    weak                    
> ._ZTH23deprecated_warning_hook
> 
> [3209]  m   0x000203d8     .data     1  unamex                    
> _ZTH23deprecated_warning_hook
> 
> Hence in the final stage of compilation while creating the binary gdb, 
> the ld error came and the compilation is unsuccessful since the symbol 
> is not visible though complaints.c file was successfully compiled.
> 
> This patch is a fix for the same where instead of thread_local we used 
> __thread and GDB now compiles in AIX. Please find attached the patch. 
> (See: 0001-Fix-AIX-build-break.patch).
> 
> I want to know your opinion about this. Also, I do not know the impact 
> of this change on other targets. If we can do this better, kindly let me 
> know.
> 
> Have a nice day ahead.
> 
> Thanks and regards,
> Aditya.
> 

Re: [PATCH] Fix AIX build break

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> I think this is triggered by using thread-local with extern.

Ugh.

>     Use RAII to set the per-thread SIGSEGV handler

Tom> which seems related, given that it mentions link errors.  Perhaps we
Tom> can do something similar here.

That would by fine with me.

Does the AIX build detect that threads work?  If not then we could
perhaps make the use of 'thread_local' dependent on CXX_STD_THREAD.

Tom

RE: [RFC] Fix AIX build break

[Aditya Kamath1]

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

Respected community members,

Hi,

Thank you so much for the feedback. We can fix this but I need your guidance as I feel I have missed something in my way to fix this for GDB.

> Does the AIX build detect that threads work?  If not then we could
>perhaps make the use of 'thread_local' dependent on CXX_STD_THREAD.

Std::thread works and CXX_STD_THREAD is set in AIX in our build Tom. Is that what you are asking for?

So I was trying to fix this issue and currently am still facing the symbol not found issue as pasted below.

ld: 0711-317 ERROR: Undefined symbol: _ZN29deprecated_warning_hook_class23 deprecated_member_funcE
collect2: error: ld returned 8 exit status
gmake: *** [Makefile:2184: gdb] Error 1

I have pasted the git diff below this email that I tried to.

As per my understanding, the deprecated_warning_hook is an extern variable declared, which is defined in top.c.

I get the idea that this link is doing https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=fece451c2aca57b095e7e4063e342781cf74aa75. Thanks Tom for this.

The design to fix this, that I have in mind is that we should declare a function pointer of deprecated hook and intialise it to NULL. This pointer which is global will be the one that will be used everywhere (named as deprecated_warning_hook in defs.h in the git diff) and this pointer will be set by only by the constructor in the class deprecated_warning_hook_class (defined in defs.h in the git diff).

So whether complaints.c wants to use this or utils.c one can define an object of this class and then through constructor one must set the pointer deprecated_warning_hook. The older pointer is held by old_handler in the class. When the object goes out of scope, the destructor will reset the deprecated_warning_hook to its previous value be it NULL or anything else.

By this way our functions will be able to use the hook and unset the same whenever out of scope. The function pointer that deprecated_warning_hook will point to deprecated_member_func which  is a private member function that is static.

This is the logic I was applying but it did not work.

Kindly let me know where I went wrong and what I missed out. Your expertise will help me fix this for GDB and learn.

Have a nice day ahead.

Thanks and regards,
Aditya.



# git diff
diff --git a/gdb/complaints.c b/gdb/complaints.c
index eb648c655ed..533aa1470a2 100644
--- a/gdb/complaints.c
+++ b/gdb/complaints.c
@@ -40,6 +40,8 @@ int stop_whining = 0;
static std::mutex complaint_mutex;
#endif /* CXX_STD_THREAD */
+deprecated_warning_hook_class deprecated_warning_hook_obj();
+
/* See complaints.h.  */
 void
diff --git a/gdb/defs.h b/gdb/defs.h
index 2f771d8dc49..5c298979614 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -562,8 +562,30 @@ extern void (*deprecated_print_frame_info_listing_hook) (struct symtab * s,
                                                         int noerror);
extern int (*deprecated_query_hook) (const char *, va_list)
      ATTRIBUTE_FPTR_PRINTF(1,0);
-extern thread_local void (*deprecated_warning_hook) (const char *, va_list)
-     ATTRIBUTE_FPTR_PRINTF(1,0);
+thread_local void (*deprecated_warning_hook) (const char *, va_list) = NULL;
+typedef void (*deprecated_warning_hook_handler) (const char *, va_list) ATTRIBUTE_FPTR_PRINTF(1,0);
+
+class deprecated_warning_hook_class
+{
+  public:
+    deprecated_warning_hook_class ();
+    ~deprecated_warning_hook_class ();
+  private:
+    deprecated_warning_hook_handler old_handler;
+       static void (*deprecated_member_func) (const char *, va_list);
+};
+
+deprecated_warning_hook_class::deprecated_warning_hook_class ()
+{
+  old_handler = deprecated_warning_hook;
+  deprecated_warning_hook = deprecated_member_func;
+}
+
+deprecated_warning_hook_class::~deprecated_warning_hook_class ()
+{
+  deprecated_warning_hook = old_handler;
+}
+
extern void (*deprecated_readline_begin_hook) (const char *, ...)
      ATTRIBUTE_FPTR_PRINTF_1;
extern char *(*deprecated_readline_hook) (const char *);
diff --git a/gdb/interps.c b/gdb/interps.c
index bec2c85e2fd..c39a5017718 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -273,7 +273,7 @@ clear_interpreter_hooks (void)
   deprecated_print_frame_info_listing_hook = 0;
   /*print_frame_more_info_hook = 0; */
   deprecated_query_hook = 0;
-  deprecated_warning_hook = 0;
+  /*deprecated_warning_hook = 0;  */
   deprecated_readline_begin_hook = 0;
   deprecated_readline_hook = 0;
   deprecated_readline_end_hook = 0;
diff --git a/gdb/top.c b/gdb/top.c
index 009bf2b0c1c..1afbdb5e19a 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -221,8 +221,6 @@ int (*deprecated_query_hook) (const char *, va_list);
 /* Replaces most of warning.  */
-thread_local void (*deprecated_warning_hook) (const char *, va_list);
-
/* These three functions support getting lines of text from the user.
    They are used in sequence.  First deprecated_readline_begin_hook is
    called with a text string that might be (for example) a message for
diff --git a/gdb/utils.c b/gdb/utils.c
index 9ae9494d51d..70db397e4fb 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -82,6 +82,8 @@
#include "pager.h"
#include "run-on-main-thread.h"
+deprecated_warning_hook_class deprecated_warning_hook_obj();
+
void (*deprecated_error_begin_hook) (void);
 /* Prototypes for local functions */

From: Tom Tromey <tom@tromey.com>
Date: Friday, 12 January 2024 at 9:26 PM
To: Tom de Vries <tdevries@suse.de>
Cc: Aditya Kamath1 <Aditya.Kamath1@ibm.com>, Ulrich Weigand <Ulrich.Weigand@de.ibm.com>, Aditya Kamath1 via Gdb-patches <gdb-patches@sourceware.org>, Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>, Tom Tromey <tom@tromey.com>
Subject: [EXTERNAL] Re: [PATCH] Fix AIX build break
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> I think this is triggered by using thread-local with extern.

Ugh.

>     Use RAII to set the per-thread SIGSEGV handler

Tom> which seems related, given that it mentions link errors.  Perhaps we
Tom> can do something similar here.

That would by fine with me.

Does the AIX build detect that threads work?  If not then we could
perhaps make the use of 'thread_local' dependent on CXX_STD_THREAD.

Tom

RE: [PATCH] Fix AIX build break

[Aditya Kamath1]

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

Respected community members,

Hi,

Please find attached the patch. (See: 0001-Fix-AIX-build-break.patch).

Thank you for your patience and feedback.

This patch now fixes the AIX build break and uses the idea as suggested by Tom in this link (https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=fece451c2aca57b095e7e4063e342781cf74aa75).

Kindly give me feedback for the same. I removed the extern thread_local and use class now.

Also, is there any guidelines for naming classes? Kindly let me know.

I am also curious to know if GDB is multi-threaded now?

Have a nice day ahead.

Thanks and regards,
Aditya.


From: Tom Tromey <tom@tromey.com>
Date: Friday, 12 January 2024 at 9:26 PM
To: Tom de Vries <tdevries@suse.de>
Cc: Aditya Kamath1 <Aditya.Kamath1@ibm.com>, Ulrich Weigand <Ulrich.Weigand@de.ibm.com>, Aditya Kamath1 via Gdb-patches <gdb-patches@sourceware.org>, Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>, Tom Tromey <tom@tromey.com>
Subject: [EXTERNAL] Re: [PATCH] Fix AIX build break
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> I think this is triggered by using thread-local with extern.

Ugh.

>     Use RAII to set the per-thread SIGSEGV handler

Tom> which seems related, given that it mentions link errors.  Perhaps we
Tom> can do something similar here.

That would by fine with me.

Does the AIX build detect that threads work?  If not then we could
perhaps make the use of 'thread_local' dependent on CXX_STD_THREAD.

Tom

[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 24153 bytes --]

Re: [PATCH] Fix AIX build break

[Tom Tromey]

>>>>> Aditya Kamath1 <Aditya.Kamath1@ibm.com> writes:

> Please find attached the patch. (See: 0001-Fix-AIX-build-break.patch).

This is in some weird format with non-ascii characters embedded.
Could you send it as text, or better yet, with 'git send-email'?

> Kindly give me feedback for the same. I removed the extern thread_local and use class now.

I think some sort of thread-locality is necessary.

> Also, is there any guidelines for naming classes? Kindly let me know.

Not really.

> I am also curious to know if GDB is multi-threaded now?

Yes.

Tom