RE: [PATCH] Fix AIX build break.

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

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