[PATCH] Fix AIX build break.

[PATCH] Fix AIX build break.

[Aditya Kamath1]

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

Respected Ulrich and GDB community members,

Hi,

In the recent commit {https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=6d30ada87b7a515a0f623687e2faadc1d4acf440} the parameters of the scoped_restore_current_inferior_for_memory function was changed and in AIX we did not update the same.

This fixed the build and threads debugging works alright.

Kindly accept this patch and commit the same. Let me know if any changes needed.

Have a nice day ahead.

Thanks and regards,
Aditya.

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

From c662a9f01e6da808351d4e92a666eeba092e8026 Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Fri, 11 Aug 2023 03:43:23 -0500
Subject: [PATCH] Fix AIX build break.

In a recent commit the signature of the scoped_restore_current_inferior_for_memory function was changed and in AIX we did not update the same.

This patch updates it in aix-thread.c file fixed the break and the thread debugging works alright as a result.
---
 gdb/aix-thread.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index 8c45116f355..1f7b3c511cf 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -615,8 +615,7 @@ pdc_read_data (pthdb_user_t user_current_pid, void *buf,
   /* This is needed to eliminate the dependency of current thread
      which is null so that thread reads the correct target memory.  */
   {
-    scoped_restore_current_inferior_for_memory save_inferior
-      (inf, ptid_t (user_current_pid));
+    scoped_restore_current_inferior_for_memory save_inferior (inf);
     status = target_read_memory (addr, (gdb_byte *) buf, len);
   }
   ret = status == 0 ? PDC_SUCCESS : PDC_FAILURE;
@@ -643,8 +642,7 @@ pdc_write_data (pthdb_user_t user_current_pid, void *buf,
 		user_current_pid, (long) buf, hex_string (addr), len);
 
   {
-    scoped_restore_current_inferior_for_memory save_inferior
-      (inf, ptid_t (user_current_pid));
+    scoped_restore_current_inferior_for_memory save_inferior (inf);
     status = target_write_memory (addr, (gdb_byte *) buf, len);
   }
 
-- 
2.38.3

Re: [PATCH] Fix AIX build break.

[Ulrich Weigand]

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

>In the recent commit {https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=6d30ada87b7a515a0f623687e2faadc1d4acf440}
>the parameters of the scoped_restore_current_inferior_for_memory function was changed and in AIX we did not update the same.
> 
>This fixed the build and threads debugging works alright.
> 
>Kindly accept this patch and commit the same. Let me know if any changes needed.

This is OK.  I've applied the patch.

Thanks,
Ulrich

Re: [PATCH] Fix AIX build break.

[Aditya Kamath1]

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

Thank you

From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Date: Friday, 11 August 2023 at 3:13 PM
To: gdb-patches@sourceware.org <gdb-patches@sourceware.org>, Aditya Kamath1 <Aditya.Kamath1@ibm.com>
Cc: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: Re: [PATCH] Fix AIX build break.
Aditya Kamath1 <Aditya.Kamath1@ibm.com> wrote:

>In the recent commit {https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=6d30ada87b7a515a0f623687e2faadc1d4acf440}
>the parameters of the scoped_restore_current_inferior_for_memory function was changed and in AIX we did not update the same.
>
>This fixed the build and threads debugging works alright.
>
>Kindly accept this patch and commit the same. Let me know if any changes needed.

This is OK.  I've applied the patch.

Thanks,
Ulrich

Re: [PATCH] Fix AIX build break.

[Aditya Kamath1]

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

Hi Ciaran and respected community members,

Thank you for reaching out.

>I did have success with the following (pasted) patch, so I can submit that
>properly if it isn't going to break anything for the AIX build, but also
>open to other suggestions.

GDB in AIX builds successfully with the patch you pasted in the previous email. We are okay with the fix.

Kindly let us know if you want to check anything else in AIX operating system.

Have a nice day ahead.

Thanks and regards,
Aditya.

From: Ciaran Woodward <ciaranwoodward@xmos.com>
Date: Tuesday, 6 February 2024 at 6:07 PM
To: Aditya Kamath1 <Aditya.Kamath1@ibm.com>, Tom Tromey <tom@tromey.com>
Cc: Tom Tromey <tom@tromey.com>, Ulrich Weigand <Ulrich.Weigand@de.ibm.com>, Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>, Aditya Kamath1 via Gdb-patches <gdb-patches@sourceware.org>
Subject: [EXTERNAL] RE: [PATCH] Fix AIX build break.
> +/* A function that can be used to intercept warnings.  */
> +typedef void (*warning_hook_handler) (const char *, va_list);
> +
> +/* Set the thread-local warning hook, and restore the old value when
> +   finished.  */
> +class scoped_restore_warning_hook
> +{
> +public:
> +  explicit scoped_restore_warning_hook (warning_hook_handler new_handler);
> +
> +private:
> +  scoped_restore_tmpl<warning_hook_handler> m_save;
> +};

This part of the patch seems to have broken the build on mingw-64 gcc 13.2.0.

I think it is because va_list on that platform has some attributes.

Example (Werror) warning:

In file included from ../../gdb/defs.h:620,
                 from ../../gdb/x86-tdep.c:20:
../../gdb/utils.h:387:43: error: ignoring attributes on template argument 'warning_hook_handler' {aka 'void (*)(const char*, char*)'} [-Werror=ignored-attributes]
  387 |   scoped_restore_tmpl<warning_hook_handler> m_save;
      |                                           ^

I'm not sure what the 'best' solution is, as we don't really care about the
type attributes in this context, as we're just saving and restoring it.

I did have success with the following (pasted) patch, so I can submit that
properly if it isn't going to break anything for the AIX build, but also
open to other suggestions.

""""""
diff --git a/gdb/utils.c b/gdb/utils.c
index 702c3f15826..9d8a6443b37 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -145,7 +145,8 @@ get_warning_hook_handler ()

 scoped_restore_warning_hook::scoped_restore_warning_hook
      (warning_hook_handler new_handler)
-       : m_save (make_scoped_restore (&warning_hook, new_handler))
+       : m_save (make_scoped_restore (reinterpret_cast<void(**)()>(&warning_hook),
+                                      reinterpret_cast<void(*)()>(new_handler)))
 {
 }

diff --git a/gdb/utils.h b/gdb/utils.h
index 7487590902a..bea19369b9b 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -384,7 +384,7 @@ class scoped_restore_warning_hook
   explicit scoped_restore_warning_hook (warning_hook_handler new_handler);

 private:
-  scoped_restore_tmpl<warning_hook_handler> m_save;
+  scoped_restore_tmpl<void(*)()> m_save;
 };

 /* Return the current warning handler.  */
""""""

Cheers,
Ciaran

RE: [PATCH] Fix AIX build break.

[Ciaran Woodward]

> What if we remove the scoped_restore use and just do the save/restore
> manually here instead?

Works for me, I'll submit a patch later.

Cheers,
Ciaran

Re: [PATCH] Fix AIX build break.

[Tom Tromey]

>>>>> "Ciaran" == Ciaran Woodward <ciaranwoodward@xmos.com> writes:

Ciaran> I think it is because va_list on that platform has some attributes.

Ugh.

Ciaran>  scoped_restore_warning_hook::scoped_restore_warning_hook
Ciaran>       (warning_hook_handler new_handler)
Ciaran> -       : m_save (make_scoped_restore (&warning_hook, new_handler))
Ciaran> +       : m_save (make_scoped_restore (reinterpret_cast<void(**)()>(&warning_hook),
Ciaran> +                                      reinterpret_cast<void(*)()>(new_handler)))

What if we remove the scoped_restore use and just do the save/restore
manually here instead?

Tom

RE: [PATCH] Fix AIX build break.

[Ciaran Woodward]

> +/* A function that can be used to intercept warnings.  */
> +typedef void (*warning_hook_handler) (const char *, va_list);
> +
> +/* Set the thread-local warning hook, and restore the old value when
> +   finished.  */
> +class scoped_restore_warning_hook
> +{
> +public:
> +  explicit scoped_restore_warning_hook (warning_hook_handler new_handler);
> +
> +private:
> +  scoped_restore_tmpl<warning_hook_handler> m_save;
> +};

This part of the patch seems to have broken the build on mingw-64 gcc 13.2.0.

I think it is because va_list on that platform has some attributes.

Example (Werror) warning:

In file included from ../../gdb/defs.h:620,
                 from ../../gdb/x86-tdep.c:20:
../../gdb/utils.h:387:43: error: ignoring attributes on template argument 'warning_hook_handler' {aka 'void (*)(const char*, char*)'} [-Werror=ignored-attributes]
  387 |   scoped_restore_tmpl<warning_hook_handler> m_save;
      |                                           ^

I'm not sure what the 'best' solution is, as we don't really care about the
type attributes in this context, as we're just saving and restoring it.

I did have success with the following (pasted) patch, so I can submit that
properly if it isn't going to break anything for the AIX build, but also
open to other suggestions.

""""""
diff --git a/gdb/utils.c b/gdb/utils.c
index 702c3f15826..9d8a6443b37 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -145,7 +145,8 @@ get_warning_hook_handler ()

 scoped_restore_warning_hook::scoped_restore_warning_hook
      (warning_hook_handler new_handler)
-       : m_save (make_scoped_restore (&warning_hook, new_handler))
+       : m_save (make_scoped_restore (reinterpret_cast<void(**)()>(&warning_hook),
+                                      reinterpret_cast<void(*)()>(new_handler)))
 {
 }

diff --git a/gdb/utils.h b/gdb/utils.h
index 7487590902a..bea19369b9b 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -384,7 +384,7 @@ class scoped_restore_warning_hook
   explicit scoped_restore_warning_hook (warning_hook_handler new_handler);

 private:
-  scoped_restore_tmpl<warning_hook_handler> m_save;
+  scoped_restore_tmpl<void(*)()> m_save;
 };

 /* Return the current warning handler.  */
""""""

Cheers,
Ciaran

RE: [PATCH] Fix AIX build break.

[Aditya Kamath1]

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

Thank you Tom.

From: Tom Tromey <tom@tromey.com>
Date: Thursday, 1 February 2024 at 6:07 AM
To: Aditya Kamath1 <Aditya.Kamath1@ibm.com>
Cc: Tom Tromey <tom@tromey.com>, Ulrich Weigand <Ulrich.Weigand@de.ibm.com>, Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>, Aditya Kamath1 via Gdb-patches <gdb-patches@sourceware.org>
Subject: [EXTERNAL] Re: [PATCH] Fix AIX build break.
>>>>> Aditya Kamath1 <Aditya.Kamath1@ibm.com> writes:

>> This looks basically reasonable, but rather than subject you to another
>> round of review, I just went ahead and made a few changes.  Could you
>> try this out?

> This patch builds AIX GDB successfully. I am okay with this.

Thanks.  I'm checking it in now.

Tom

Re: [PATCH] Fix AIX build break.

[Tom Tromey]

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

>> This looks basically reasonable, but rather than subject you to another
>> round of review, I just went ahead and made a few changes.  Could you
>> try this out?
 
> This patch builds AIX GDB successfully. I am okay with this. 

Thanks.  I'm checking it in now.

Tom

RE: [PATCH] Fix AIX build break.

[Aditya Kamath1]

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

Respected Tom and community members,

Hi,

>This looks basically reasonable, but rather than subject you to another
>round of review, I just went ahead and made a few changes.  Could you
>try this out?

This patch builds AIX GDB successfully. I am okay with this.

>Basically there were some formatting issues and I thought the code ought
>to be in utils.c.
>+private:
>+  scoped_restore_tmpl<warning_hook_handler> m_save;

This is nice.

Thank you so much for your guidance. Kindly commit the patch version you pasted in the previous email.

>You may wish to send an email to the Insight list, warning them that
>this is going to break their build.

Sure. Will send an email to the insight list.

Have a nice day ahead.

Thanks and regards,
Aditya.


From: Tom Tromey <tom@tromey.com>
Date: Wednesday, 31 January 2024 at 6:11 AM
To: Aditya Kamath1 <Aditya.Kamath1@ibm.com>
Cc: Tom Tromey <tom@tromey.com>, Ulrich Weigand <Ulrich.Weigand@de.ibm.com>, Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>, Aditya Kamath1 via Gdb-patches <gdb-patches@sourceware.org>
Subject: [EXTERNAL] Re: [PATCH] Fix AIX build break.
>>>>> "Aditya" == Aditya Kamath1 <Aditya.Kamath1@ibm.com> writes:

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

Aditya> Thank you for your feedback.

Thanks.

This looks basically reasonable, but rather than subject you to another
round of review, I just went ahead and made a few changes.  Could you
try this out?

Basically there were some formatting issues and I thought the code ought
to be in utils.c.

You may wish to send an email to the Insight list, warning them that
this is going to break their build.

Tom

From c67ddecdef06043a916a07ad34b3f4db090d377f Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Fri, 26 Jan 2024 02:19:52 -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.

This patch is a fix for the same.
---
 gdb/complaints.c | 16 +++++++++-------
 gdb/complaints.h |  8 +++-----
 gdb/defs.h       |  2 --
 gdb/interps.c    |  1 -
 gdb/top.c        |  4 ----
 gdb/utils.c      | 23 +++++++++++++++++++++--
 gdb/utils.h      | 17 +++++++++++++++++
 7 files changed, 50 insertions(+), 21 deletions(-)

diff --git a/gdb/complaints.c b/gdb/complaints.c
index 0c46cd7c84f..496736fe95f 100644
--- a/gdb/complaints.c
+++ b/gdb/complaints.c
@@ -57,8 +57,9 @@ complaint_internal (const char *fmt, ...)

   va_start (args, fmt);

-  if (deprecated_warning_hook)
-    (*deprecated_warning_hook) (fmt, args);
+  warning_hook_handler handler = get_warning_hook_handler ();
+  if (handler != nullptr)
+    handler (fmt, args);
   else
     {
       gdb_puts (_("During symbol reading: "), gdb_stderr);
@@ -84,15 +85,15 @@ thread_local complaint_interceptor *complaint_interceptor::g_complaint_intercept
 /* See complaints.h.  */

 complaint_interceptor::complaint_interceptor ()
-  : m_saved_warning_hook (&deprecated_warning_hook, issue_complaint),
-    m_saved_complaint_interceptor (&g_complaint_interceptor, this)
+  : m_saved_complaint_interceptor (&g_complaint_interceptor, this),
+    m_saved_warning_hook (issue_complaint)
 {
 }

 /* A helper that wraps a warning hook.  */

 static void
-wrap_warning_hook (void (*hook) (const char *, va_list), ...)
+wrap_warning_hook (warning_hook_handler hook, ...)
 {
   va_list args;
   va_start (args, hook);
@@ -109,8 +110,9 @@ re_emit_complaints (const complaint_collection &complaints)

   for (const std::string &str : complaints)
     {
-      if (deprecated_warning_hook)
-       wrap_warning_hook (deprecated_warning_hook, str.c_str ());
+      warning_hook_handler handler = get_warning_hook_handler ();
+      if (handler != nullptr)
+       wrap_warning_hook (handler, str.c_str ());
       else
         gdb_printf (gdb_stderr, _("During symbol reading: %s\n"),
                     str.c_str ());
diff --git a/gdb/complaints.h b/gdb/complaints.h
index baf10fd7dd7..8ac4c5034ee 100644
--- a/gdb/complaints.h
+++ b/gdb/complaints.h
@@ -89,11 +89,6 @@ class complaint_interceptor
   /* The issued complaints.  */
   complaint_collection m_complaints;

-  typedef void (*saved_warning_hook_ftype) (const char *, va_list);
-
-  /* The saved value of deprecated_warning_hook.  */
-  scoped_restore_tmpl<saved_warning_hook_ftype> m_saved_warning_hook;
-
   /* The saved value of g_complaint_interceptor.  */
   scoped_restore_tmpl<complaint_interceptor *> m_saved_complaint_interceptor;

@@ -104,6 +99,9 @@ class complaint_interceptor

   /* This object.  Used by the static callback function.  */
   static thread_local complaint_interceptor *g_complaint_interceptor;
+
+  /* Object to initialise the warning hook.  */
+  scoped_restore_warning_hook m_saved_warning_hook;
 };

 /* Re-emit complaints that were collected by complaint_interceptor.
diff --git a/gdb/defs.h b/gdb/defs.h
index f4664000641..cf471bf5d66 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -562,8 +562,6 @@ 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);
 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 eddc7f3c871..391fea1da03 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -273,7 +273,6 @@ 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_readline_begin_hook = 0;
   deprecated_readline_hook = 0;
   deprecated_readline_end_hook = 0;
diff --git a/gdb/top.c b/gdb/top.c
index fb15c719564..5114713baa4 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -219,10 +219,6 @@ void (*deprecated_print_frame_info_listing_hook) (struct symtab * s,

 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 b326033e9ff..702c3f15826 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -128,7 +128,26 @@ show_pagination_enabled (struct ui_file *file, int from_tty,
 }


+/* Warning hook pointer.  This has to be 'static' to avoid link
+   problems with thread-locals on AIX.  */

+static thread_local void (*warning_hook) (const char *, va_list);
+
+/* See utils.h.  */
+
+warning_hook_handler
+get_warning_hook_handler ()
+{
+  return warning_hook;
+}
+
+/* See utils.h.  */
+
+scoped_restore_warning_hook::scoped_restore_warning_hook
+     (warning_hook_handler new_handler)
+       : m_save (make_scoped_restore (&warning_hook, new_handler))
+{
+}

 /* Print a warning message.  The first argument STRING is the warning
    message, used as an fprintf format string, the second is the
@@ -139,8 +158,8 @@ show_pagination_enabled (struct ui_file *file, int from_tty,
 void
 vwarning (const char *string, va_list args)
 {
-  if (deprecated_warning_hook)
-    (*deprecated_warning_hook) (string, args);
+  if (warning_hook != nullptr)
+    warning_hook (string, args);
   else
     {
       std::optional<target_terminal::scoped_restore_terminal_state> term_state;
diff --git a/gdb/utils.h b/gdb/utils.h
index a9d04746b21..7487590902a 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -373,6 +373,23 @@ assign_return_if_changed (T &lval, const T &val)
   return true;
 }

+/* A function that can be used to intercept warnings.  */
+typedef void (*warning_hook_handler) (const char *, va_list);
+
+/* Set the thread-local warning hook, and restore the old value when
+   finished.  */
+class scoped_restore_warning_hook
+{
+public:
+  explicit scoped_restore_warning_hook (warning_hook_handler new_handler);
+
+private:
+  scoped_restore_tmpl<warning_hook_handler> m_save;
+};
+
+/* Return the current warning handler.  */
+extern warning_hook_handler get_warning_hook_handler ();
+
 /* In some cases GDB needs to try several different solutions to a problem,
    if any of the solutions work then as far as the user is concerned the
    problem is solved, and GDB should continue without warnings.  However,
--
2.43.0

Re: [PATCH] Fix AIX build break.

[Tom Tromey]

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

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

Aditya> Thank you for your feedback. 

Thanks.

This looks basically reasonable, but rather than subject you to another
round of review, I just went ahead and made a few changes.  Could you
try this out?

Basically there were some formatting issues and I thought the code ought
to be in utils.c.

You may wish to send an email to the Insight list, warning them that
this is going to break their build.

Tom

From c67ddecdef06043a916a07ad34b3f4db090d377f Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Fri, 26 Jan 2024 02:19:52 -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.

This patch is a fix for the same.
---
 gdb/complaints.c | 16 +++++++++-------
 gdb/complaints.h |  8 +++-----
 gdb/defs.h       |  2 --
 gdb/interps.c    |  1 -
 gdb/top.c        |  4 ----
 gdb/utils.c      | 23 +++++++++++++++++++++--
 gdb/utils.h      | 17 +++++++++++++++++
 7 files changed, 50 insertions(+), 21 deletions(-)

diff --git a/gdb/complaints.c b/gdb/complaints.c
index 0c46cd7c84f..496736fe95f 100644
--- a/gdb/complaints.c
+++ b/gdb/complaints.c
@@ -57,8 +57,9 @@ complaint_internal (const char *fmt, ...)
 
   va_start (args, fmt);
 
-  if (deprecated_warning_hook)
-    (*deprecated_warning_hook) (fmt, args);
+  warning_hook_handler handler = get_warning_hook_handler ();
+  if (handler != nullptr)
+    handler (fmt, args);
   else
     {
       gdb_puts (_("During symbol reading: "), gdb_stderr);
@@ -84,15 +85,15 @@ thread_local complaint_interceptor *complaint_interceptor::g_complaint_intercept
 /* See complaints.h.  */
 
 complaint_interceptor::complaint_interceptor ()
-  : m_saved_warning_hook (&deprecated_warning_hook, issue_complaint),
-    m_saved_complaint_interceptor (&g_complaint_interceptor, this)
+  : m_saved_complaint_interceptor (&g_complaint_interceptor, this),
+    m_saved_warning_hook (issue_complaint)
 {
 }
 
 /* A helper that wraps a warning hook.  */
 
 static void
-wrap_warning_hook (void (*hook) (const char *, va_list), ...)
+wrap_warning_hook (warning_hook_handler hook, ...)
 {
   va_list args;
   va_start (args, hook);
@@ -109,8 +110,9 @@ re_emit_complaints (const complaint_collection &complaints)
 
   for (const std::string &str : complaints)
     {
-      if (deprecated_warning_hook)
-	wrap_warning_hook (deprecated_warning_hook, str.c_str ());
+      warning_hook_handler handler = get_warning_hook_handler ();
+      if (handler != nullptr)
+	wrap_warning_hook (handler, str.c_str ());
       else
 	gdb_printf (gdb_stderr, _("During symbol reading: %s\n"),
 		    str.c_str ());
diff --git a/gdb/complaints.h b/gdb/complaints.h
index baf10fd7dd7..8ac4c5034ee 100644
--- a/gdb/complaints.h
+++ b/gdb/complaints.h
@@ -89,11 +89,6 @@ class complaint_interceptor
   /* The issued complaints.  */
   complaint_collection m_complaints;
 
-  typedef void (*saved_warning_hook_ftype) (const char *, va_list);
-
-  /* The saved value of deprecated_warning_hook.  */
-  scoped_restore_tmpl<saved_warning_hook_ftype> m_saved_warning_hook;
-
   /* The saved value of g_complaint_interceptor.  */
   scoped_restore_tmpl<complaint_interceptor *> m_saved_complaint_interceptor;
 
@@ -104,6 +99,9 @@ class complaint_interceptor
 
   /* This object.  Used by the static callback function.  */
   static thread_local complaint_interceptor *g_complaint_interceptor;
+
+  /* Object to initialise the warning hook.  */
+  scoped_restore_warning_hook m_saved_warning_hook;
 };
 
 /* Re-emit complaints that were collected by complaint_interceptor.
diff --git a/gdb/defs.h b/gdb/defs.h
index f4664000641..cf471bf5d66 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -562,8 +562,6 @@ 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);
 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 eddc7f3c871..391fea1da03 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -273,7 +273,6 @@ 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_readline_begin_hook = 0;
   deprecated_readline_hook = 0;
   deprecated_readline_end_hook = 0;
diff --git a/gdb/top.c b/gdb/top.c
index fb15c719564..5114713baa4 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -219,10 +219,6 @@ void (*deprecated_print_frame_info_listing_hook) (struct symtab * s,
 
 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 b326033e9ff..702c3f15826 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -128,7 +128,26 @@ show_pagination_enabled (struct ui_file *file, int from_tty,
 }
 
 \f
+/* Warning hook pointer.  This has to be 'static' to avoid link
+   problems with thread-locals on AIX.  */
 
+static thread_local void (*warning_hook) (const char *, va_list);
+
+/* See utils.h.  */
+
+warning_hook_handler
+get_warning_hook_handler ()
+{
+  return warning_hook;
+}
+
+/* See utils.h.  */
+
+scoped_restore_warning_hook::scoped_restore_warning_hook
+     (warning_hook_handler new_handler)
+       : m_save (make_scoped_restore (&warning_hook, new_handler))
+{
+}
 
 /* Print a warning message.  The first argument STRING is the warning
    message, used as an fprintf format string, the second is the
@@ -139,8 +158,8 @@ show_pagination_enabled (struct ui_file *file, int from_tty,
 void
 vwarning (const char *string, va_list args)
 {
-  if (deprecated_warning_hook)
-    (*deprecated_warning_hook) (string, args);
+  if (warning_hook != nullptr)
+    warning_hook (string, args);
   else
     {
       std::optional<target_terminal::scoped_restore_terminal_state> term_state;
diff --git a/gdb/utils.h b/gdb/utils.h
index a9d04746b21..7487590902a 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -373,6 +373,23 @@ assign_return_if_changed (T &lval, const T &val)
   return true;
 }
 
+/* A function that can be used to intercept warnings.  */
+typedef void (*warning_hook_handler) (const char *, va_list);
+
+/* Set the thread-local warning hook, and restore the old value when
+   finished.  */
+class scoped_restore_warning_hook
+{
+public:
+  explicit scoped_restore_warning_hook (warning_hook_handler new_handler);
+
+private:
+  scoped_restore_tmpl<warning_hook_handler> m_save;
+};
+
+/* Return the current warning handler.  */
+extern warning_hook_handler get_warning_hook_handler ();
+
 /* In some cases GDB needs to try several different solutions to a problem,
    if any of the solutions work then as far as the user is concerned the
    problem is solved, and GDB should continue without warnings.  However,
-- 
2.43.0

RE: [PATCH] Fix AIX build break.

[Aditya Kamath1]

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

Respected Tom and community members,

Hi,

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

Thank you for your feedback.

>> +#include "warning-header.h"
>This header has a static definition, but this means the symbol will be
>created twice -- once at each include.  However, this is wrong.

Okay. Hmm. Can we use a function interface (If I can call so) defined in defs.h in this version of the patch by which any other files which needs the pointer to work with can access?

>> +scoped_restore_warning_hook::scoped_restore_warning_hook (warning_hook_handler new_handler)
>Line too long.
>> +scoped_restore_warning_hook::~scoped_restore_warning_hook()
>Space before '('.
>> +class scoped_restore_warning_hook
>> +{
>> +  public:
>Indentation here looks wrong I think?
>> +  private:
>> +    warning_hook_handler old_handler;
>gdb convention is to name private members with "m_" prefix.

Fixed all these in this version of the patch.

>> +static void (*deprecated_warning_hook) (const char *, va_list);
>> +
>> +#endif
>It's bad to define things in headers.  There should be just a single
>definition in a .c file, and this still has to be thread-local.

Please check now.

>> +{
>> +  deprecated_warning_hook = old_handler;
>> +}
>This class can just wrap a scoped_restore.

I did not get you. Looking around did you mean we should use something like scoped_restore pstate_restore = make_scoped_restore (&pstate);??

Kindly let me know what you think of this version of the patch. I am thankful for your patience while I learn how to solve this for GDB.

Have a nice day ahead.

Thanks and regards,
Aditya.

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

> +#include "warning-header.h"

This header has a static definition, but this means the symbol will be
created twice -- once at each include.  However, this is wrong.

> +
> +scoped_restore_warning_hook::scoped_restore_warning_hook (warning_hook_handler new_handler)

Line too long.

> +scoped_restore_warning_hook::~scoped_restore_warning_hook()

Space before '('.

> +{
> +  deprecated_warning_hook = old_handler;
> +}

This class can just wrap a scoped_restore.

> +class scoped_restore_warning_hook
> +{
> +  public:

Indentation here looks wrong I think?

> +  private:
> +    warning_hook_handler old_handler;

gdb convention is to name private members with "m_" prefix.

> diff --git a/gdb/warning-header.h b/gdb/warning-header.h
> new file mode 100644
> index 00000000000..14acf9bcb9b
> --- /dev/null
> +++ b/gdb/warning-header.h
> @@ -0,0 +1,6 @@
> +#ifndef GDB_WARNING_HOOK
> +#define GDB_WARNING_HOOK 1
> +
> +static void (*deprecated_warning_hook) (const char *, va_list);
> +
> +#endif

It's bad to define things in headers.  There should be just a single
definition in a .c file, and this still has to be thread-local.

Tom

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

From b4421e786d8fdde2b7edd828c71bbc50a4962b76 Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Fri, 26 Jan 2024 02:19:52 -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.

This patch is a fix for the same.
---
 gdb/complaints.c | 39 +++++++++++++++++++++++++++++++++------
 gdb/complaints.h |  8 +++-----
 gdb/defs.h       | 15 +++++++++++++--
 gdb/interps.c    |  2 +-
 gdb/top.c        |  4 ----
 gdb/utils.c      |  4 ++--
 6 files changed, 52 insertions(+), 20 deletions(-)

diff --git a/gdb/complaints.c b/gdb/complaints.c
index 0c46cd7c84f..8cd2d16ec03 100644
--- a/gdb/complaints.c
+++ b/gdb/complaints.c
@@ -26,6 +26,10 @@
 #include <unordered_map>
 #include <mutex>
 
+/* warning hook pointer.  */
+
+static thread_local void (*warning_hook) (const char *, va_list);
+
 /* Map format strings to counters.  */
 
 static std::unordered_map<const char *, int> counters;
@@ -40,6 +44,29 @@ int stop_whining = 0;
 static std::mutex complaint_mutex;
 #endif /* CXX_STD_THREAD */
 
+/* This function is to get the hook pointer value in other files
+   wherever used.  For example vwarning () in utils.c.  */
+
+warning_hook_handler get_warning_hook_handler ()
+{
+  return warning_hook? warning_hook: nullptr;
+}
+
+/* The constructor and destructor to take care of restoring the
+   warning hook once the scoped_restore_warning hook is note needed.  */
+
+scoped_restore_warning_hook::scoped_restore_warning_hook
+(warning_hook_handler new_handler)
+{
+  m_old_handler = warning_hook;
+  warning_hook = new_handler;
+}
+
+scoped_restore_warning_hook::~scoped_restore_warning_hook ()
+{
+  warning_hook = m_old_handler;
+}
+
 /* See complaints.h.  */
 
 void
@@ -57,8 +84,8 @@ complaint_internal (const char *fmt, ...)
 
   va_start (args, fmt);
 
-  if (deprecated_warning_hook)
-    (*deprecated_warning_hook) (fmt, args);
+  if (warning_hook)
+    (*warning_hook) (fmt, args);
   else
     {
       gdb_puts (_("During symbol reading: "), gdb_stderr);
@@ -84,8 +111,8 @@ thread_local complaint_interceptor *complaint_interceptor::g_complaint_intercept
 /* See complaints.h.  */
 
 complaint_interceptor::complaint_interceptor ()
-  : m_saved_warning_hook (&deprecated_warning_hook, issue_complaint),
-    m_saved_complaint_interceptor (&g_complaint_interceptor, this)
+  : m_saved_complaint_interceptor (&g_complaint_interceptor, this),
+    m_saved_warning_hook (issue_complaint)
 {
 }
 
@@ -109,8 +136,8 @@ re_emit_complaints (const complaint_collection &complaints)
 
   for (const std::string &str : complaints)
     {
-      if (deprecated_warning_hook)
-	wrap_warning_hook (deprecated_warning_hook, str.c_str ());
+      if (warning_hook)
+	wrap_warning_hook (warning_hook, str.c_str ());
       else
 	gdb_printf (gdb_stderr, _("During symbol reading: %s\n"),
 		    str.c_str ());
diff --git a/gdb/complaints.h b/gdb/complaints.h
index baf10fd7dd7..8ac4c5034ee 100644
--- a/gdb/complaints.h
+++ b/gdb/complaints.h
@@ -89,11 +89,6 @@ class complaint_interceptor
   /* The issued complaints.  */
   complaint_collection m_complaints;
 
-  typedef void (*saved_warning_hook_ftype) (const char *, va_list);
-
-  /* The saved value of deprecated_warning_hook.  */
-  scoped_restore_tmpl<saved_warning_hook_ftype> m_saved_warning_hook;
-
   /* The saved value of g_complaint_interceptor.  */
   scoped_restore_tmpl<complaint_interceptor *> m_saved_complaint_interceptor;
 
@@ -104,6 +99,9 @@ class complaint_interceptor
 
   /* This object.  Used by the static callback function.  */
   static thread_local complaint_interceptor *g_complaint_interceptor;
+
+  /* Object to initialise the warning hook.  */
+  scoped_restore_warning_hook m_saved_warning_hook;
 };
 
 /* Re-emit complaints that were collected by complaint_interceptor.
diff --git a/gdb/defs.h b/gdb/defs.h
index f4664000641..e78ccd6a698 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -116,6 +116,19 @@ using RequireLongest = gdb::Requires<gdb::Or<std::is_same<T, LONGEST>,
 
 #include "hashtab.h"
 
+typedef void (*warning_hook_handler) (const char *, va_list);
+
+class scoped_restore_warning_hook
+{
+public:
+  explicit scoped_restore_warning_hook (warning_hook_handler new_handler);
+  ~scoped_restore_warning_hook ();
+private:
+  warning_hook_handler m_old_handler;
+};
+
+warning_hook_handler get_warning_hook_handler ();
+
 /* * System root path, used to find libraries etc.  */
 extern std::string gdb_sysroot;
 
@@ -562,8 +575,6 @@ 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);
 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 eddc7f3c871..2d34186da27 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 fb15c719564..5114713baa4 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -219,10 +219,6 @@ void (*deprecated_print_frame_info_listing_hook) (struct symtab * s,
 
 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 b326033e9ff..0b3558c98f5 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -139,8 +139,8 @@ show_pagination_enabled (struct ui_file *file, int from_tty,
 void
 vwarning (const char *string, va_list args)
 {
-  if (deprecated_warning_hook)
-    (*deprecated_warning_hook) (string, args);
+  if (get_warning_hook_handler ())
+    (*get_warning_hook_handler ()) (string, args);
   else
     {
       std::optional<target_terminal::scoped_restore_terminal_state> term_state;
-- 
2.41.0

Re: [PATCH] Fix AIX build break.

[Tom Tromey]

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

> +#include "warning-header.h"

This header has a static definition, but this means the symbol will be
created twice -- once at each include.  However, this is wrong.

> +
> +scoped_restore_warning_hook::scoped_restore_warning_hook (warning_hook_handler new_handler)

Line too long.

> +scoped_restore_warning_hook::~scoped_restore_warning_hook()

Space before '('.

> +{
> +  deprecated_warning_hook = old_handler;
> +}
 
This class can just wrap a scoped_restore.

> +class scoped_restore_warning_hook
> +{
> +  public:

Indentation here looks wrong I think?

> +  private:
> +    warning_hook_handler old_handler;

gdb convention is to name private members with "m_" prefix.

> diff --git a/gdb/warning-header.h b/gdb/warning-header.h
> new file mode 100644
> index 00000000000..14acf9bcb9b
> --- /dev/null
> +++ b/gdb/warning-header.h
> @@ -0,0 +1,6 @@
> +#ifndef GDB_WARNING_HOOK
> +#define GDB_WARNING_HOOK 1
> +
> +static void (*deprecated_warning_hook) (const char *, va_list);
> +
> +#endif

It's bad to define things in headers.  There should be just a single
definition in a .c file, and this still has to be thread-local.

Tom

RE: [PATCH] Fix AIX build break.

[Aditya Kamath1]

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

Respected community members,

Kindly give me feedback on the proposed patch fix in the previous email. I am attaching it again. (See: 0001-Fix-AIX-build-break.patch).

AIX internal CI build for GDB has been broken for some time now. Your feedback will help us fix it and get the build back to be successful. I appreciate your patience, feedback and apologise for any inconvenience.

Have a nice day ahead.

Thanks and regards,
Aditya.
From: Aditya Kamath1 <Aditya.Kamath1@ibm.com>
Date: Wednesday, 24 January 2024 at 6:23 AM
To: Tom Tromey <tom@tromey.com>
Cc: Tom Tromey <tom@tromey.com>, gdb-patches@sourceware.org <gdb-patches@sourceware.org>, Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: [EXTERNAL] RE: [PATCH] Fix AIX build break.
Respected community members, Kindly give me feedback on the proposed patch fix in the previous email. I am attaching it again. (See: 0001-Fix-AIX-build-break. patch). Thank you for your feedback so far. I appreciate the same. Have a nice day
ZjQcmQRYFpfptBannerStart
This Message Is From an External Sender
This message came from outside your organization.
Report Suspicious <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/PjiDSg!1e-vrb4zRvm6FYv7uCHFAORWk7v3HWsHs9s34qJckfJKaItUCLcGOM5pVI488NA5bZmsODgR0ctKErKNlz9v1TMuDE-CYpiUrYUXVjNCh3L6HKtbAJ1pMYlP0ZA_keER-NXr5PMQZHfm9Q$>


ZjQcmQRYFpfptBannerEnd
Respected community members,

Kindly give me feedback on the proposed patch fix in the previous email. I am attaching it again. (See: 0001-Fix-AIX-build-break.patch).

Thank you for your feedback so far. I appreciate the same.

Have a nice day ahead.

Thanks and regards,
Aditya.

-----------------------------------------------------

From: Aditya Kamath1 <Aditya.Kamath1@ibm.com>
Date: Saturday, 20 January 2024 at 8:34 PM
To: Tom Tromey <tom@tromey.com>
Cc: Tom Tromey <tom@tromey.com>, gdb-patches@sourceware.org <gdb-patches@sourceware.org>, Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: [EXTERNAL] RE: [PATCH] Fix AIX build break.
Respected Tom and community members, Hi, Please find attached a patch. (See: 0001-Fix-AIX-build-break. patch). Thank you for your guidance and patience. I have renamed the class to scoped_. Let’s keep it the same as rest of GDB. >This is
ZjQcmQRYFpfptBannerStart
This Message Is From an External Sender
This message came from outside your organization.
Report Suspicious<https://us-phishalarm-ewt.proofpoint.com/EWT/v1/PjiDSg!1e-vj57TRvm6FYv7uCHFgLMIRsJ6mxx0BC_rBlL-YBJvJ9G2nNcZja6eDYZyvuXujIGphNcgsQB_Ycaw9E7NnTeP22lGbTBdKkgv2McvvfAE2vtUQANKno-QGODoIojgMLw4qvW9hG4rww$>


ZjQcmQRYFpfptBannerEnd
Respected Tom and community members,

Hi,

Please find attached a patch. (See: 0001-Fix-AIX-build-break.patch). Thank you for your guidance and patience.

I have renamed the class to scoped_. Let’s keep it the same as rest of GDB.

>This is static in utils.c now, so it means that nothing else can affect it.
>However this changes how the interception works -- it disables it.  But,
>the reason the interception is done is to avoid emitting warnings from
>worker threads.

I get this. I got the fact that my patch in my previous email was horribly wrong.

Let me explain you the design in this patch.

So in the code in 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 see we are setting the deprecated_warning_hook pointer to issue_complaint function and this is the only place where I see the pointer is set. If this pointer is set then we use this function pointer and its required parameters to display the warning via wrap_warning_hook ().

One fact is that in the commit deprecated_warning_hook is declared as an extern thread_local in defs.h, perhaps defined in top.c and used in three places complaints.c, utils.c and interps.c. extern declaration made is easy to use it in three files but caused this undefined symbol error issue.

So we need to ensure that we do not use extern and at the same time the deprecated_warning_hook pointer is declared only once and used by all the three files based on whether the pointer is nullptr or set to a function address.

Inorder to do the same, I used a new header file warning-header.h and the below lines in it to ensure we declare it only once.
+#ifndef GDB_WARNING_HOOK
+#define GDB_WARNING_HOOK 1
+
+static void (*deprecated_warning_hook) (const char *, va_list);
+
+#endif

This header file is now included all the three files namely utils.c, complaints.c and interps.c.

The class scoped_restore_warning_hook defined in defs.h is incharge of setting and restoring the function pointer whenever required. This object of this class is declared in the complaint_interceptor and initialised in complaints.c. Now we will have the function pointer set. So any file or thread (since the g_complaint_interceptor pointer is thread_local) who is using this function pointer deprecated_warning_hook can comfortably recognise if this pointer is set or not and can execute. Also, the destructor will ensure the pointer is back to where its original value.

The only thing is why we need a new header file. So, I was searching for a header file that is common amongst the three files and I found it was defs.h. But the moment I declared the function pointer in the same style as warning-header.h many files importing defs.h complained about unused variable deprecated_warning_hook.

Do suggest me if you have better alternatives for the pointer declaration or we can use some other header file accessible by all the three files or any other way. I would love to understand and implement.

So this is the whole idea. I am thankful for your guidance to help me so that we can fix this. It took me time to figure this. Let me know if I am still wrong somewhere.

Kindly give me a feedback for the same.

Have a nice day ahead.

Thanks and regards,
Aditya.

From: Tom Tromey <tom@tromey.com>
Date: Friday, 19 January 2024 at 9:55 PM
To: Aditya Kamath1 <Aditya.Kamath1@ibm.com>
Cc: Tom Tromey <tom@tromey.com>, gdb-patches@sourceware.org <gdb-patches@sourceware.org>, Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: [EXTERNAL] Re: [PATCH] Fix AIX build break.
> The only thing I am unsure of is the utils.c file where we also use
> the deprecated_warning_hook. Should we use an object of
> warning_hook_class there as well?

I'll add some notes inline.  I don't think this patch can really work.

> From the experiments I conducted this is true. It is because
> g_complaint_interceptor is a static thread_local variable and not used
> anywhere else we do not see it as an unreferenced symbol in AIX.

Don't be deceived by the 'static' keyword.  C++ overloads this.  This
variable is in fact extern.

> +static thread_local void (*deprecated_warning_hook) (const char *, va_list);

This is defined in two places.  This is confusing at best, but the one
here (in complaints.c) means that warning_hook_class won't interoperate
properly with the other global.  This seems very weird.

> +warning_hook_class::warning_hook_class (warning_hook_handler new_handler)

I wouldn't put '_class' in a class name.
It's more normal in gdb to prefix scoped things with "scoped_", but
other choices are possible.

> diff --git a/gdb/utils.c b/gdb/utils.c
> index b326033e9ff..23530c2a41f 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -82,6 +82,8 @@
>  #include "pager.h"
>  #include "run-on-main-thread.h"

> +static thread_local void (*deprecated_warning_hook) (const char *, va_list);
> +

This is static in utils.c now, so it means that nothing else can affect it.
However this changes how the interception works -- it disables it.  But,
the reason the interception is done is to avoid emitting warnings from
worker threads.

Tom

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

From 7785f0d61009a53d40b51ebceb51b7f53a8deafa Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Sat, 20 Jan 2024 08:28:48 -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.

This patch is a fix for the same.
---
 gdb/complaints.c     | 16 ++++++++++++++--
 gdb/complaints.h     |  8 +++-----
 gdb/defs.h           | 13 +++++++++++--
 gdb/top.c            |  4 ----
 gdb/utils.c          |  1 +
 gdb/warning-header.h |  6 ++++++
 6 files changed, 35 insertions(+), 13 deletions(-)
 create mode 100644 gdb/warning-header.h

diff --git a/gdb/complaints.c b/gdb/complaints.c
index 0c46cd7c84f..8a949e1754f 100644
--- a/gdb/complaints.c
+++ b/gdb/complaints.c
@@ -25,6 +25,18 @@
 #include "gdbsupport/selftest.h"
 #include <unordered_map>
 #include <mutex>
+#include "warning-header.h"
+
+scoped_restore_warning_hook::scoped_restore_warning_hook (warning_hook_handler new_handler)
+{
+  old_handler = deprecated_warning_hook;
+  deprecated_warning_hook = new_handler;
+}
+
+scoped_restore_warning_hook::~scoped_restore_warning_hook()
+{
+  deprecated_warning_hook = old_handler;
+}
 
 /* Map format strings to counters.  */
 
@@ -84,8 +96,8 @@ thread_local complaint_interceptor *complaint_interceptor::g_complaint_intercept
 /* See complaints.h.  */
 
 complaint_interceptor::complaint_interceptor ()
-  : m_saved_warning_hook (&deprecated_warning_hook, issue_complaint),
-    m_saved_complaint_interceptor (&g_complaint_interceptor, this)
+    : m_saved_complaint_interceptor (&g_complaint_interceptor, this),
+      m_saved_warning_hook (issue_complaint)
 {
 }
 
diff --git a/gdb/complaints.h b/gdb/complaints.h
index baf10fd7dd7..8ac4c5034ee 100644
--- a/gdb/complaints.h
+++ b/gdb/complaints.h
@@ -89,11 +89,6 @@ class complaint_interceptor
   /* The issued complaints.  */
   complaint_collection m_complaints;
 
-  typedef void (*saved_warning_hook_ftype) (const char *, va_list);
-
-  /* The saved value of deprecated_warning_hook.  */
-  scoped_restore_tmpl<saved_warning_hook_ftype> m_saved_warning_hook;
-
   /* The saved value of g_complaint_interceptor.  */
   scoped_restore_tmpl<complaint_interceptor *> m_saved_complaint_interceptor;
 
@@ -104,6 +99,9 @@ class complaint_interceptor
 
   /* This object.  Used by the static callback function.  */
   static thread_local complaint_interceptor *g_complaint_interceptor;
+
+  /* Object to initialise the warning hook.  */
+  scoped_restore_warning_hook m_saved_warning_hook;
 };
 
 /* Re-emit complaints that were collected by complaint_interceptor.
diff --git a/gdb/defs.h b/gdb/defs.h
index f4664000641..32961b5314d 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -116,6 +116,17 @@ using RequireLongest = gdb::Requires<gdb::Or<std::is_same<T, LONGEST>,
 
 #include "hashtab.h"
 
+typedef void (*warning_hook_handler) (const char *, va_list);
+
+class scoped_restore_warning_hook
+{
+  public:
+    explicit scoped_restore_warning_hook (warning_hook_handler new_handler);
+    ~scoped_restore_warning_hook ();
+  private:
+    warning_hook_handler old_handler;
+};
+
 /* * System root path, used to find libraries etc.  */
 extern std::string gdb_sysroot;
 
@@ -562,8 +573,6 @@ 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);
 extern void (*deprecated_readline_begin_hook) (const char *, ...)
      ATTRIBUTE_FPTR_PRINTF_1;
 extern char *(*deprecated_readline_hook) (const char *);
diff --git a/gdb/top.c b/gdb/top.c
index fb15c719564..5114713baa4 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -219,10 +219,6 @@ void (*deprecated_print_frame_info_listing_hook) (struct symtab * s,
 
 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 b326033e9ff..5290f105100 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -81,6 +81,7 @@
 #include "gdbsupport/buildargv.h"
 #include "pager.h"
 #include "run-on-main-thread.h"
+#include "warning-header.h"
 
 void (*deprecated_error_begin_hook) (void);
 
diff --git a/gdb/warning-header.h b/gdb/warning-header.h
new file mode 100644
index 00000000000..14acf9bcb9b
--- /dev/null
+++ b/gdb/warning-header.h
@@ -0,0 +1,6 @@
+#ifndef GDB_WARNING_HOOK
+#define GDB_WARNING_HOOK 1
+
+static void (*deprecated_warning_hook) (const char *, va_list);
+
+#endif
-- 
2.41.0

RE: [PATCH] Fix AIX build break.

[Aditya Kamath1]

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

Respected community members,

Kindly give me feedback on the proposed patch fix in the previous email. I am attaching it again. (See: 0001-Fix-AIX-build-break.patch).

Thank you for your feedback so far. I appreciate the same.

Have a nice day ahead.

Thanks and regards,
Aditya.

-----------------------------------------------------

From: Aditya Kamath1 <Aditya.Kamath1@ibm.com>
Date: Saturday, 20 January 2024 at 8:34 PM
To: Tom Tromey <tom@tromey.com>
Cc: Tom Tromey <tom@tromey.com>, gdb-patches@sourceware.org <gdb-patches@sourceware.org>, Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: [EXTERNAL] RE: [PATCH] Fix AIX build break.
Respected Tom and community members, Hi, Please find attached a patch. (See: 0001-Fix-AIX-build-break. patch). Thank you for your guidance and patience. I have renamed the class to scoped_. Let’s keep it the same as rest of GDB. >This is
ZjQcmQRYFpfptBannerStart
This Message Is From an External Sender
This message came from outside your organization.
Report Suspicious <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/PjiDSg!1e-vj57TRvm6FYv7uCHFgLMIRsJ6mxx0BC_rBlL-YBJvJ9G2nNcZja6eDYZyvuXujIGphNcgsQB_Ycaw9E7NnTeP22lGbTBdKkgv2McvvfAE2vtUQANKno-QGODoIojgMLw4qvW9hG4rww$>


ZjQcmQRYFpfptBannerEnd
Respected Tom and community members,

Hi,

Please find attached a patch. (See: 0001-Fix-AIX-build-break.patch). Thank you for your guidance and patience.

I have renamed the class to scoped_. Let’s keep it the same as rest of GDB.

>This is static in utils.c now, so it means that nothing else can affect it.
>However this changes how the interception works -- it disables it.  But,
>the reason the interception is done is to avoid emitting warnings from
>worker threads.

I get this. I got the fact that my patch in my previous email was horribly wrong.

Let me explain you the design in this patch.

So in the code in 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 see we are setting the deprecated_warning_hook pointer to issue_complaint function and this is the only place where I see the pointer is set. If this pointer is set then we use this function pointer and its required parameters to display the warning via wrap_warning_hook ().

One fact is that in the commit deprecated_warning_hook is declared as an extern thread_local in defs.h, perhaps defined in top.c and used in three places complaints.c, utils.c and interps.c. extern declaration made is easy to use it in three files but caused this undefined symbol error issue.

So we need to ensure that we do not use extern and at the same time the deprecated_warning_hook pointer is declared only once and used by all the three files based on whether the pointer is nullptr or set to a function address.

Inorder to do the same, I used a new header file warning-header.h and the below lines in it to ensure we declare it only once.
+#ifndef GDB_WARNING_HOOK
+#define GDB_WARNING_HOOK 1
+
+static void (*deprecated_warning_hook) (const char *, va_list);
+
+#endif

This header file is now included all the three files namely utils.c, complaints.c and interps.c.

The class scoped_restore_warning_hook defined in defs.h is incharge of setting and restoring the function pointer whenever required. This object of this class is declared in the complaint_interceptor and initialised in complaints.c. Now we will have the function pointer set. So any file or thread (since the g_complaint_interceptor pointer is thread_local) who is using this function pointer deprecated_warning_hook can comfortably recognise if this pointer is set or not and can execute. Also, the destructor will ensure the pointer is back to where its original value.

The only thing is why we need a new header file. So, I was searching for a header file that is common amongst the three files and I found it was defs.h. But the moment I declared the function pointer in the same style as warning-header.h many files importing defs.h complained about unused variable deprecated_warning_hook.

Do suggest me if you have better alternatives for the pointer declaration or we can use some other header file accessible by all the three files or any other way. I would love to understand and implement.

So this is the whole idea. I am thankful for your guidance to help me so that we can fix this. It took me time to figure this. Let me know if I am still wrong somewhere.

Kindly give me a feedback for the same.

Have a nice day ahead.

Thanks and regards,
Aditya.

From: Tom Tromey <tom@tromey.com>
Date: Friday, 19 January 2024 at 9:55 PM
To: Aditya Kamath1 <Aditya.Kamath1@ibm.com>
Cc: Tom Tromey <tom@tromey.com>, gdb-patches@sourceware.org <gdb-patches@sourceware.org>, Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: [EXTERNAL] Re: [PATCH] Fix AIX build break.
> The only thing I am unsure of is the utils.c file where we also use
> the deprecated_warning_hook. Should we use an object of
> warning_hook_class there as well?

I'll add some notes inline.  I don't think this patch can really work.

> From the experiments I conducted this is true. It is because
> g_complaint_interceptor is a static thread_local variable and not used
> anywhere else we do not see it as an unreferenced symbol in AIX.

Don't be deceived by the 'static' keyword.  C++ overloads this.  This
variable is in fact extern.

> +static thread_local void (*deprecated_warning_hook) (const char *, va_list);

This is defined in two places.  This is confusing at best, but the one
here (in complaints.c) means that warning_hook_class won't interoperate
properly with the other global.  This seems very weird.

> +warning_hook_class::warning_hook_class (warning_hook_handler new_handler)

I wouldn't put '_class' in a class name.
It's more normal in gdb to prefix scoped things with "scoped_", but
other choices are possible.

> diff --git a/gdb/utils.c b/gdb/utils.c
> index b326033e9ff..23530c2a41f 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -82,6 +82,8 @@
>  #include "pager.h"
>  #include "run-on-main-thread.h"

> +static thread_local void (*deprecated_warning_hook) (const char *, va_list);
> +

This is static in utils.c now, so it means that nothing else can affect it.
However this changes how the interception works -- it disables it.  But,
the reason the interception is done is to avoid emitting warnings from
worker threads.

Tom

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

From 7785f0d61009a53d40b51ebceb51b7f53a8deafa Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Sat, 20 Jan 2024 08:28:48 -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.

This patch is a fix for the same.
---
 gdb/complaints.c     | 16 ++++++++++++++--
 gdb/complaints.h     |  8 +++-----
 gdb/defs.h           | 13 +++++++++++--
 gdb/top.c            |  4 ----
 gdb/utils.c          |  1 +
 gdb/warning-header.h |  6 ++++++
 6 files changed, 35 insertions(+), 13 deletions(-)
 create mode 100644 gdb/warning-header.h

diff --git a/gdb/complaints.c b/gdb/complaints.c
index 0c46cd7c84f..8a949e1754f 100644
--- a/gdb/complaints.c
+++ b/gdb/complaints.c
@@ -25,6 +25,18 @@
 #include "gdbsupport/selftest.h"
 #include <unordered_map>
 #include <mutex>
+#include "warning-header.h"
+
+scoped_restore_warning_hook::scoped_restore_warning_hook (warning_hook_handler new_handler)
+{
+  old_handler = deprecated_warning_hook;
+  deprecated_warning_hook = new_handler;
+}
+
+scoped_restore_warning_hook::~scoped_restore_warning_hook()
+{
+  deprecated_warning_hook = old_handler;
+}
 
 /* Map format strings to counters.  */
 
@@ -84,8 +96,8 @@ thread_local complaint_interceptor *complaint_interceptor::g_complaint_intercept
 /* See complaints.h.  */
 
 complaint_interceptor::complaint_interceptor ()
-  : m_saved_warning_hook (&deprecated_warning_hook, issue_complaint),
-    m_saved_complaint_interceptor (&g_complaint_interceptor, this)
+    : m_saved_complaint_interceptor (&g_complaint_interceptor, this),
+      m_saved_warning_hook (issue_complaint)
 {
 }
 
diff --git a/gdb/complaints.h b/gdb/complaints.h
index baf10fd7dd7..8ac4c5034ee 100644
--- a/gdb/complaints.h
+++ b/gdb/complaints.h
@@ -89,11 +89,6 @@ class complaint_interceptor
   /* The issued complaints.  */
   complaint_collection m_complaints;
 
-  typedef void (*saved_warning_hook_ftype) (const char *, va_list);
-
-  /* The saved value of deprecated_warning_hook.  */
-  scoped_restore_tmpl<saved_warning_hook_ftype> m_saved_warning_hook;
-
   /* The saved value of g_complaint_interceptor.  */
   scoped_restore_tmpl<complaint_interceptor *> m_saved_complaint_interceptor;
 
@@ -104,6 +99,9 @@ class complaint_interceptor
 
   /* This object.  Used by the static callback function.  */
   static thread_local complaint_interceptor *g_complaint_interceptor;
+
+  /* Object to initialise the warning hook.  */
+  scoped_restore_warning_hook m_saved_warning_hook;
 };
 
 /* Re-emit complaints that were collected by complaint_interceptor.
diff --git a/gdb/defs.h b/gdb/defs.h
index f4664000641..32961b5314d 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -116,6 +116,17 @@ using RequireLongest = gdb::Requires<gdb::Or<std::is_same<T, LONGEST>,
 
 #include "hashtab.h"
 
+typedef void (*warning_hook_handler) (const char *, va_list);
+
+class scoped_restore_warning_hook
+{
+  public:
+    explicit scoped_restore_warning_hook (warning_hook_handler new_handler);
+    ~scoped_restore_warning_hook ();
+  private:
+    warning_hook_handler old_handler;
+};
+
 /* * System root path, used to find libraries etc.  */
 extern std::string gdb_sysroot;
 
@@ -562,8 +573,6 @@ 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);
 extern void (*deprecated_readline_begin_hook) (const char *, ...)
      ATTRIBUTE_FPTR_PRINTF_1;
 extern char *(*deprecated_readline_hook) (const char *);
diff --git a/gdb/top.c b/gdb/top.c
index fb15c719564..5114713baa4 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -219,10 +219,6 @@ void (*deprecated_print_frame_info_listing_hook) (struct symtab * s,
 
 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 b326033e9ff..5290f105100 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -81,6 +81,7 @@
 #include "gdbsupport/buildargv.h"
 #include "pager.h"
 #include "run-on-main-thread.h"
+#include "warning-header.h"
 
 void (*deprecated_error_begin_hook) (void);
 
diff --git a/gdb/warning-header.h b/gdb/warning-header.h
new file mode 100644
index 00000000000..14acf9bcb9b
--- /dev/null
+++ b/gdb/warning-header.h
@@ -0,0 +1,6 @@
+#ifndef GDB_WARNING_HOOK
+#define GDB_WARNING_HOOK 1
+
+static void (*deprecated_warning_hook) (const char *, va_list);
+
+#endif
-- 
2.41.0

RE: [PATCH] Fix AIX build break.

[Aditya Kamath1]

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

Respected Tom and community members,

Hi,

Please find attached a patch. (See: 0001-Fix-AIX-build-break.patch). Thank you for your guidance and patience.

I have renamed the class to scoped_. Let’s keep it the same as rest of GDB.

>This is static in utils.c now, so it means that nothing else can affect it.
>However this changes how the interception works -- it disables it.  But,
>the reason the interception is done is to avoid emitting warnings from
>worker threads.

I get this. I got the fact that my patch in my previous email was horribly wrong.

Let me explain you the design in this patch.

So in the code in the commit (https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=54b815ddb428944a70694e3767a0fadbdd9ca9ea) I see we are setting the deprecated_warning_hook pointer to issue_complaint function and this is the only place where I see the pointer is set. If this pointer is set then we use this function pointer and its required parameters to display the warning via wrap_warning_hook ().

One fact is that in the commit deprecated_warning_hook is declared as an extern thread_local in defs.h, perhaps defined in top.c and used in three places complaints.c, utils.c and interps.c. extern declaration made is easy to use it in three files but caused this undefined symbol error issue.

So we need to ensure that we do not use extern and at the same time the deprecated_warning_hook pointer is declared only once and used by all the three files based on whether the pointer is nullptr or set to a function address.

Inorder to do the same, I used a new header file warning-header.h and the below lines in it to ensure we declare it only once.
+#ifndef GDB_WARNING_HOOK
+#define GDB_WARNING_HOOK 1
+
+static void (*deprecated_warning_hook) (const char *, va_list);
+
+#endif

This header file is now included all the three files namely utils.c, complaints.c and interps.c.

The class scoped_restore_warning_hook defined in defs.h is incharge of setting and restoring the function pointer whenever required. This object of this class is declared in the complaint_interceptor and initialised in complaints.c. Now we will have the function pointer set. So any file or thread (since the g_complaint_interceptor pointer is thread_local) who is using this function pointer deprecated_warning_hook can comfortably recognise if this pointer is set or not and can execute. Also, the destructor will ensure the pointer is back to where its original value.

The only thing is why we need a new header file. So, I was searching for a header file that is common amongst the three files and I found it was defs.h. But the moment I declared the function pointer in the same style as warning-header.h many files importing defs.h complained about unused variable deprecated_warning_hook.

Do suggest me if you have better alternatives for the pointer declaration or we can use some other header file accessible by all the three files or any other way. I would love to understand and implement.

So this is the whole idea. I am thankful for your guidance to help me so that we can fix this. It took me time to figure this. Let me know if I am still wrong somewhere.

Kindly give me a feedback for the same.

Have a nice day ahead.

Thanks and regards,
Aditya.

From: Tom Tromey <tom@tromey.com>
Date: Friday, 19 January 2024 at 9:55 PM
To: Aditya Kamath1 <Aditya.Kamath1@ibm.com>
Cc: Tom Tromey <tom@tromey.com>, gdb-patches@sourceware.org <gdb-patches@sourceware.org>, Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: [EXTERNAL] Re: [PATCH] Fix AIX build break.
> The only thing I am unsure of is the utils.c file where we also use
> the deprecated_warning_hook. Should we use an object of
> warning_hook_class there as well?

I'll add some notes inline.  I don't think this patch can really work.

> From the experiments I conducted this is true. It is because
> g_complaint_interceptor is a static thread_local variable and not used
> anywhere else we do not see it as an unreferenced symbol in AIX.

Don't be deceived by the 'static' keyword.  C++ overloads this.  This
variable is in fact extern.

> +static thread_local void (*deprecated_warning_hook) (const char *, va_list);

This is defined in two places.  This is confusing at best, but the one
here (in complaints.c) means that warning_hook_class won't interoperate
properly with the other global.  This seems very weird.

> +warning_hook_class::warning_hook_class (warning_hook_handler new_handler)

I wouldn't put '_class' in a class name.
It's more normal in gdb to prefix scoped things with "scoped_", but
other choices are possible.

> diff --git a/gdb/utils.c b/gdb/utils.c
> index b326033e9ff..23530c2a41f 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -82,6 +82,8 @@
>  #include "pager.h"
>  #include "run-on-main-thread.h"

> +static thread_local void (*deprecated_warning_hook) (const char *, va_list);
> +

This is static in utils.c now, so it means that nothing else can affect it.
However this changes how the interception works -- it disables it.  But,
the reason the interception is done is to avoid emitting warnings from
worker threads.

Tom

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

From 7785f0d61009a53d40b51ebceb51b7f53a8deafa Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Sat, 20 Jan 2024 08:28:48 -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.

This patch is a fix for the same.
---
 gdb/complaints.c     | 16 ++++++++++++++--
 gdb/complaints.h     |  8 +++-----
 gdb/defs.h           | 13 +++++++++++--
 gdb/top.c            |  4 ----
 gdb/utils.c          |  1 +
 gdb/warning-header.h |  6 ++++++
 6 files changed, 35 insertions(+), 13 deletions(-)
 create mode 100644 gdb/warning-header.h

diff --git a/gdb/complaints.c b/gdb/complaints.c
index 0c46cd7c84f..8a949e1754f 100644
--- a/gdb/complaints.c
+++ b/gdb/complaints.c
@@ -25,6 +25,18 @@
 #include "gdbsupport/selftest.h"
 #include <unordered_map>
 #include <mutex>
+#include "warning-header.h"
+
+scoped_restore_warning_hook::scoped_restore_warning_hook (warning_hook_handler new_handler)
+{
+  old_handler = deprecated_warning_hook;
+  deprecated_warning_hook = new_handler;
+}
+
+scoped_restore_warning_hook::~scoped_restore_warning_hook()
+{
+  deprecated_warning_hook = old_handler;
+}
 
 /* Map format strings to counters.  */
 
@@ -84,8 +96,8 @@ thread_local complaint_interceptor *complaint_interceptor::g_complaint_intercept
 /* See complaints.h.  */
 
 complaint_interceptor::complaint_interceptor ()
-  : m_saved_warning_hook (&deprecated_warning_hook, issue_complaint),
-    m_saved_complaint_interceptor (&g_complaint_interceptor, this)
+    : m_saved_complaint_interceptor (&g_complaint_interceptor, this),
+      m_saved_warning_hook (issue_complaint)
 {
 }
 
diff --git a/gdb/complaints.h b/gdb/complaints.h
index baf10fd7dd7..8ac4c5034ee 100644
--- a/gdb/complaints.h
+++ b/gdb/complaints.h
@@ -89,11 +89,6 @@ class complaint_interceptor
   /* The issued complaints.  */
   complaint_collection m_complaints;
 
-  typedef void (*saved_warning_hook_ftype) (const char *, va_list);
-
-  /* The saved value of deprecated_warning_hook.  */
-  scoped_restore_tmpl<saved_warning_hook_ftype> m_saved_warning_hook;
-
   /* The saved value of g_complaint_interceptor.  */
   scoped_restore_tmpl<complaint_interceptor *> m_saved_complaint_interceptor;
 
@@ -104,6 +99,9 @@ class complaint_interceptor
 
   /* This object.  Used by the static callback function.  */
   static thread_local complaint_interceptor *g_complaint_interceptor;
+
+  /* Object to initialise the warning hook.  */
+  scoped_restore_warning_hook m_saved_warning_hook;
 };
 
 /* Re-emit complaints that were collected by complaint_interceptor.
diff --git a/gdb/defs.h b/gdb/defs.h
index f4664000641..32961b5314d 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -116,6 +116,17 @@ using RequireLongest = gdb::Requires<gdb::Or<std::is_same<T, LONGEST>,
 
 #include "hashtab.h"
 
+typedef void (*warning_hook_handler) (const char *, va_list);
+
+class scoped_restore_warning_hook
+{
+  public:
+    explicit scoped_restore_warning_hook (warning_hook_handler new_handler);
+    ~scoped_restore_warning_hook ();
+  private:
+    warning_hook_handler old_handler;
+};
+
 /* * System root path, used to find libraries etc.  */
 extern std::string gdb_sysroot;
 
@@ -562,8 +573,6 @@ 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);
 extern void (*deprecated_readline_begin_hook) (const char *, ...)
      ATTRIBUTE_FPTR_PRINTF_1;
 extern char *(*deprecated_readline_hook) (const char *);
diff --git a/gdb/top.c b/gdb/top.c
index fb15c719564..5114713baa4 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -219,10 +219,6 @@ void (*deprecated_print_frame_info_listing_hook) (struct symtab * s,
 
 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 b326033e9ff..5290f105100 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -81,6 +81,7 @@
 #include "gdbsupport/buildargv.h"
 #include "pager.h"
 #include "run-on-main-thread.h"
+#include "warning-header.h"
 
 void (*deprecated_error_begin_hook) (void);
 
diff --git a/gdb/warning-header.h b/gdb/warning-header.h
new file mode 100644
index 00000000000..14acf9bcb9b
--- /dev/null
+++ b/gdb/warning-header.h
@@ -0,0 +1,6 @@
+#ifndef GDB_WARNING_HOOK
+#define GDB_WARNING_HOOK 1
+
+static void (*deprecated_warning_hook) (const char *, va_list);
+
+#endif
-- 
2.41.0

Re: [PATCH] Fix AIX build break.

[Tom Tromey]

> The only thing I am unsure of is the utils.c file where we also use
> the deprecated_warning_hook. Should we use an object of
> warning_hook_class there as well?

I'll add some notes inline.  I don't think this patch can really work.
 
> From the experiments I conducted this is true. It is because
> g_complaint_interceptor is a static thread_local variable and not used
> anywhere else we do not see it as an unreferenced symbol in AIX.

Don't be deceived by the 'static' keyword.  C++ overloads this.  This
variable is in fact extern.

> +static thread_local void (*deprecated_warning_hook) (const char *, va_list);

This is defined in two places.  This is confusing at best, but the one
here (in complaints.c) means that warning_hook_class won't interoperate
properly with the other global.  This seems very weird.

> +warning_hook_class::warning_hook_class (warning_hook_handler new_handler)

I wouldn't put '_class' in a class name.
It's more normal in gdb to prefix scoped things with "scoped_", but
other choices are possible.

> diff --git a/gdb/utils.c b/gdb/utils.c
> index b326033e9ff..23530c2a41f 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -82,6 +82,8 @@
>  #include "pager.h"
>  #include "run-on-main-thread.h"
 
> +static thread_local void (*deprecated_warning_hook) (const char *, va_list);
> +
 
This is static in utils.c now, so it means that nothing else can affect it.
However this changes how the interception works -- it disables it.  But,
the reason the interception is done is to avoid emitting warnings from
worker threads.

Tom

RE: [PATCH] Fix AIX build break.

[Aditya Kamath1]

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

Respected Tom and community members,

Hi,

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

Thank you for your guidance and patience. I appreciate the same. I had got this solution wrong in my previous patch.

So we will be using the complaint_interceptor class to create the warning_hook_class object and initialising like how it is done in complaints.c file as done in this patch.

The only thing I am unsure of is the utils.c file where we also use the deprecated_warning_hook. Should we use an object of warning_hook_class there as well?

Kindly let me know if this version of the patch needs more changes.

>> +  public:
>> +    deprecated_warning_hook_class (deprecated_warning_hook_handler new_handler);

>Single-arg constructors should ordinarily be 'explicit'.

>complaints.h should instantiate the new class in complaint_interceptor.

This is done.

>> +typedef void (*deprecated_warning_hook_handler) (const char *, va_list) ATTRIBUTE_FPTR_PRINTF(1,0);

>Should probably remove the equivalent typedef from complaints.h.

>It seems to me that we're using this hook more now, so removing the
>'deprecated' prefix seems appropriate

I have made these changes.

>I'm curious to know why complaint_interceptor::g_complaint_interceptor
>doesn't cause this same problem.  It seems the same to me.  Is it
>because it isn't referenced anywhere other than complaints.c?

From the experiments I conducted this is true. It is because g_complaint_interceptor is a static thread_local variable and not used anywhere else we do not see it as an unreferenced symbol in AIX.

Whenever we use an extern thread_local variable in AIX and compile with GCC, we get that variable as an unreferenced symbol and hence the error. For example, say we have two files; one files named as file1.cpp with static thread_local foo =1 and file2.cpp with extern thread_local foo declared and simply printing it. (file2.cpp uses the foo variable of file1.cpp). In this case as well we see, AIX returning an unreferenced symbol foo. So this extern thread_local declarations are causing the problem in AIX. Kindly let me know if you need more information or I misunderstood the question you asked.

Have a nice day ahead.

Thanks and regards,
Aditya.
From: Tom Tromey <tom@tromey.com>
Date: Thursday, 18 January 2024 at 12:55 AM
To: Aditya Kamath1 <Aditya.Kamath1@ibm.com>
Cc: tom@tromey.com <tom@tromey.com>, gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Subject: [EXTERNAL] Re: [PATCH] Fix AIX build break.
>>>>> Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com> writes:

Thanks for the patch.

> +/* local deprecated warning hook.  */
> +
> +void (*local_warning_hook) (const char *, va_list);

I don't understand what this is for.

> @@ -56,7 +64,6 @@ complaint_internal (const char *fmt, ...)
>    }

>    va_start (args, fmt);
> -
>    if (deprecated_warning_hook)
>      (*deprecated_warning_hook) (fmt, args);
>    else

Spurious change.

> +static thread_local void (*deprecated_warning_hook) (const char *, va_list) = nullptr;

Putting this in the header is wrong.
This means that there will be multiple instances of this variable.

> +typedef void (*deprecated_warning_hook_handler) (const char *, va_list) ATTRIBUTE_FPTR_PRINTF(1,0);

Should probably remove the equivalent typedef from complaints.h.

It seems to me that we're using this hook more now, so removing the
'deprecated' prefix seems appropriate.  That will mess with Insight but
this patch is already going to require them to change.

> +
> +class deprecated_warning_hook_class
> +{
> +  public:
> +    deprecated_warning_hook_class (deprecated_warning_hook_handler new_handler);

Single-arg constructors should ordinarily be 'explicit'.

complaints.h should instantiate the new class in complaint_interceptor.
The approach in this patch won't really work, I think.  The idea is that
the hook should be temporarily (i.e., scoped) set in worker threads.

The clear_interpreter_hooks hunk changes the semantics, though I'm not
really sure that the current code is correct either.  It seems bizarre
to me.

I'm curious to know why complaint_interceptor::g_complaint_interceptor
doesn't cause this same problem.  It seems the same to me.  Is it
because it isn't referenced anywhere other than complaints.c?

Tom

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

From 579840e3547d82d886cccd70aed1eabab8187896 Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Fri, 19 Jan 2024 05:20:17 -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.

This patch is a fix for the same.
---
 gdb/complaints.c | 15 ++++++++++++++-
 gdb/complaints.h |  7 ++++---
 gdb/defs.h       | 13 +++++++++++--
 gdb/interps.c    |  2 +-
 gdb/top.c        |  4 ----
 gdb/utils.c      |  2 ++
 6 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/gdb/complaints.c b/gdb/complaints.c
index 0c46cd7c84f..b8af486eb59 100644
--- a/gdb/complaints.c
+++ b/gdb/complaints.c
@@ -26,6 +26,18 @@
 #include <unordered_map>
 #include <mutex>
 
+static thread_local void (*deprecated_warning_hook) (const char *, va_list);
+warning_hook_class::warning_hook_class (warning_hook_handler new_handler)
+{
+  old_handler = deprecated_warning_hook;
+  deprecated_warning_hook = new_handler;
+}
+
+warning_hook_class::~warning_hook_class ()
+{
+  deprecated_warning_hook = old_handler;
+}
+
 /* Map format strings to counters.  */
 
 static std::unordered_map<const char *, int> counters;
@@ -85,7 +97,8 @@ thread_local complaint_interceptor *complaint_interceptor::g_complaint_intercept
 
 complaint_interceptor::complaint_interceptor ()
   : m_saved_warning_hook (&deprecated_warning_hook, issue_complaint),
-    m_saved_complaint_interceptor (&g_complaint_interceptor, this)
+    m_saved_complaint_interceptor (&g_complaint_interceptor, this),
+    obj (deprecated_warning_hook)
 {
 }
 
diff --git a/gdb/complaints.h b/gdb/complaints.h
index baf10fd7dd7..fcd02080886 100644
--- a/gdb/complaints.h
+++ b/gdb/complaints.h
@@ -89,10 +89,8 @@ class complaint_interceptor
   /* The issued complaints.  */
   complaint_collection m_complaints;
 
-  typedef void (*saved_warning_hook_ftype) (const char *, va_list);
-
   /* The saved value of deprecated_warning_hook.  */
-  scoped_restore_tmpl<saved_warning_hook_ftype> m_saved_warning_hook;
+  scoped_restore_tmpl<warning_hook_handler> m_saved_warning_hook;
 
   /* The saved value of g_complaint_interceptor.  */
   scoped_restore_tmpl<complaint_interceptor *> m_saved_complaint_interceptor;
@@ -104,6 +102,9 @@ class complaint_interceptor
 
   /* This object.  Used by the static callback function.  */
   static thread_local complaint_interceptor *g_complaint_interceptor;
+
+  /* Object to initialise the warning hook.  */
+  warning_hook_class obj;
 };
 
 /* Re-emit complaints that were collected by complaint_interceptor.
diff --git a/gdb/defs.h b/gdb/defs.h
index f4664000641..88391436594 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -116,6 +116,17 @@ using RequireLongest = gdb::Requires<gdb::Or<std::is_same<T, LONGEST>,
 
 #include "hashtab.h"
 
+typedef void (*warning_hook_handler) (const char *, va_list);
+
+class warning_hook_class
+{
+  public:
+    explicit warning_hook_class (warning_hook_handler new_handler);
+    ~warning_hook_class ();
+  private:
+    warning_hook_handler old_handler;
+};
+
 /* * System root path, used to find libraries etc.  */
 extern std::string gdb_sysroot;
 
@@ -562,8 +573,6 @@ 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);
 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 eddc7f3c871..2d34186da27 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 fb15c719564..5114713baa4 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -219,10 +219,6 @@ void (*deprecated_print_frame_info_listing_hook) (struct symtab * s,
 
 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 b326033e9ff..23530c2a41f 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -82,6 +82,8 @@
 #include "pager.h"
 #include "run-on-main-thread.h"
 
+static thread_local void (*deprecated_warning_hook) (const char *, va_list);
+
 void (*deprecated_error_begin_hook) (void);
 
 /* Prototypes for local functions */
-- 
2.41.0

Re: [PATCH] Fix AIX build break.

[Tom Tromey]

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

Thanks for the patch.

> +/* local deprecated warning hook.  */
> +
> +void (*local_warning_hook) (const char *, va_list);

I don't understand what this is for.

> @@ -56,7 +64,6 @@ complaint_internal (const char *fmt, ...)
>    }
 
>    va_start (args, fmt);
> -
>    if (deprecated_warning_hook)
>      (*deprecated_warning_hook) (fmt, args);
>    else

Spurious change.

> +static thread_local void (*deprecated_warning_hook) (const char *, va_list) = nullptr;

Putting this in the header is wrong.
This means that there will be multiple instances of this variable.

> +typedef void (*deprecated_warning_hook_handler) (const char *, va_list) ATTRIBUTE_FPTR_PRINTF(1,0);

Should probably remove the equivalent typedef from complaints.h.

It seems to me that we're using this hook more now, so removing the
'deprecated' prefix seems appropriate.  That will mess with Insight but
this patch is already going to require them to change.

> +
> +class deprecated_warning_hook_class
> +{
> +  public:
> +    deprecated_warning_hook_class (deprecated_warning_hook_handler new_handler);

Single-arg constructors should ordinarily be 'explicit'.

complaints.h should instantiate the new class in complaint_interceptor.
The approach in this patch won't really work, I think.  The idea is that
the hook should be temporarily (i.e., scoped) set in worker threads.

The clear_interpreter_hooks hunk changes the semantics, though I'm not
really sure that the current code is correct either.  It seems bizarre
to me.

I'm curious to know why complaint_interceptor::g_complaint_interceptor
doesn't cause this same problem.  It seems the same to me.  Is it
because it isn't referenced anywhere other than complaints.c?

Tom

[PATCH] Fix AIX build break.

[Aditya Vidyadhar Kamath]

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.

This patch is a fix for the same.
---
 gdb/complaints.c |  9 ++++++++-
 gdb/defs.h       | 26 ++++++++++++++++++++++++--
 gdb/interps.c    |  1 -
 gdb/top.c        |  4 ----
 gdb/utils.c      |  8 ++++++++
 5 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/gdb/complaints.c b/gdb/complaints.c
index eb648c655ed..3a5d775b216 100644
--- a/gdb/complaints.c
+++ b/gdb/complaints.c
@@ -26,6 +26,14 @@
 #include <unordered_map>
 #include <mutex>
 
+/* local deprecated warning hook.  */
+
+void (*local_warning_hook) (const char *, va_list);
+
+/* Object for warning hook.  */
+
+deprecated_warning_hook_class obj (deprecated_warning_hook? local_warning_hook: nullptr);
+
 /* Map format strings to counters.  */
 
 static std::unordered_map<const char *, int> counters;
@@ -56,7 +64,6 @@ complaint_internal (const char *fmt, ...)
   }
 
   va_start (args, fmt);
-
   if (deprecated_warning_hook)
     (*deprecated_warning_hook) (fmt, args);
   else
diff --git a/gdb/defs.h b/gdb/defs.h
index 2f771d8dc49..b132c552d17 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);
+
+static thread_local void (*deprecated_warning_hook) (const char *, va_list) = nullptr;
+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_handler new_handler);
+    ~deprecated_warning_hook_class ();
+  private:
+    deprecated_warning_hook_handler old_handler;
+};
+
+deprecated_warning_hook_class::deprecated_warning_hook_class (deprecated_warning_hook_handler new_handler)
+{
+  old_handler = deprecated_warning_hook;
+  deprecated_warning_hook = new_handler;
+}
+
+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..1d8d05de858 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -273,7 +273,6 @@ 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_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..5708e530684 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -219,10 +219,6 @@ void (*deprecated_print_frame_info_listing_hook) (struct symtab * s,
 
 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..c15790aea6a 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -82,6 +82,14 @@
 #include "pager.h"
 #include "run-on-main-thread.h"
 
+/* local warning hook.  */
+
+void (*local_warning_hook) (const char *, va_list);
+
+/* Object for warning hook.  */
+
+deprecated_warning_hook_class deprecated_warning_hook_obj(local_warning_hook);
+
 void (*deprecated_error_begin_hook) (void);
 
 /* Prototypes for local functions */
-- 
2.41.0

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

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]

>>>>> "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

[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.
> 

[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