RE: [PATCH] Fix AIX build break.

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

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