global-file-syms for clang

global-file-syms for clang

[grant]

I need to get the global-file-syms option operation on the clang plugin. 
Is this already done?

The reason for the ask is the project I'm working on has the same 
filename used and the linker is complaining about redefinition of the 
_annobin_{filename}_start.

Regards,
Grant Haidinyak

Re: global-file-syms for clang

[Nick Clifton]

Hi Grant,

> I need to get the global-file-syms option operation on the clang plugin. Is this already done?

No. :-(

> The reason for the ask is the project I'm working on has the same filename used and the linker is complaining about redefinition of the _annobin_{filename}_start.

Ah yes....

You could use gcc as a workaround.  (Just being cheeky).

I will ask one of my colleagues to have a look at the issue, since
he has been making other enhancements to the clang plugin recently.

Cheers
   Nick

Re: global-file-syms for clang

[grant]

Nick,

Thanks. As a data point, when I use the GCC toolchain, I didn't need to 
set the global-file-syms flag. I believe the issue is an interaction 
between the LLVM gold plugin and the annobin plugin and linking in LTO 
archives that have objects that have the same filename for the source. 
The LLVM gold plugin tries to bring in all the fine annobin symbols, 
rather than just bringing the annobin symbols from the first file that 
satisfies the symbols for linking.

Disabling one of the above constraints (annobin plugin, LTO) causes the 
build to complete.

I was just going to use the global-file-syms to overcome the LLVM gold 
plugin/LTO issue.

Regards,
Grant Haidinyak

On 2024-03-21 01:35, Nick Clifton wrote:
> Hi Grant,
> 
>> I need to get the global-file-syms option operation on the clang 
>> plugin. Is this already done?
> 
> No. :-(
> 
>> The reason for the ask is the project I'm working on has the same 
>> filename used and the linker is complaining about redefinition of the 
>> _annobin_{filename}_start.
> 
> Ah yes....
> 
> You could use gcc as a workaround.  (Just being cheeky).
> 
> I will ask one of my colleagues to have a look at the issue, since
> he has been making other enhancements to the clang plugin recently.
> 
> Cheers
>   Nick

Re: global-file-syms for clang

[Tulio Magno Quites Machado Filho]

grant@hcubed.com writes:

> Thanks. As a data point, when I use the GCC toolchain, I didn't need to 
> set the global-file-syms flag. I believe the issue is an interaction 
> between the LLVM gold plugin and the annobin plugin and linking in LTO 
> archives that have objects that have the same filename for the source. 
> The LLVM gold plugin tries to bring in all the fine annobin symbols, 
> rather than just bringing the annobin symbols from the first file that 
> satisfies the symbols for linking.
>
> Disabling one of the above constraints (annobin plugin, LTO) causes the 
> build to complete.

Thank you! This information was important to reproduce the issue here.

It also happens when using lld.

-- 
Tulio Magno

[PATCH] Add support for global-file-syms to the clang and llvm plugins

[Tulio Magno Quites Machado Filho]

From: Tulio Magno Quites Machado Filho <tuliom@redhat.com>

This part starts by moving the implementation of parse_env() to
annobin-common.cc in order to be shared between the gcc and the llvm
plugins.

This was required because the llvm plugin didn't have a mechanism to
receive arguments from the user. This commit adds support for using the
environment variable ANNOBIN for the LLVM plugin, although it's still
missing many of the basic features available in the other plugins.

Both clang and llvm plugins are now able to generate the same output as
the GCC plugin, e.g. _annobin_hello_c_1711138217_00204218_start.
---
 annobin-common.cc        | 61 ++++++++++++++++++++++++++++++++++++++++
 annobin-common.h         | 19 +++++++++++++
 clang-plugin/Makefile.in | 12 +++++++-
 clang-plugin/annobin.cpp | 45 +++++++++++++++++++++++++----
 gcc-plugin/Makefile.am   |  2 +-
 gcc-plugin/Makefile.in   |  7 +++--
 gcc-plugin/annobin.cc    | 45 ++---------------------------
 llvm-plugin/Makefile.in  | 21 ++++++++++++--
 llvm-plugin/annobin.cpp  | 53 ++++++++++++++++++++++++++++++++--
 9 files changed, 208 insertions(+), 57 deletions(-)
 create mode 100644 annobin-common.cc
 create mode 100644 annobin-common.h

diff --git a/annobin-common.cc b/annobin-common.cc
new file mode 100644
index 0000000..bbdba41
--- /dev/null
+++ b/annobin-common.cc
@@ -0,0 +1,61 @@
+/* annobin - Common functions used by plugins.
+   Copyright (c) 2024 Red Hat.
+
+  This is free software; you can redistribute it and/or modify it
+  under the terms of the GNU General Public License as published
+  by the Free Software Foundation; either version 3, or (at your
+  option) any later version.
+
+  It is distributed in the hope that it will be useful, but
+  WITHOUT ANY WARRANTY; without even the implied warranty of
+  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+  GNU General Public License for more details.  */
+
+#include "annobin-common.h"
+
+#include <stdlib.h>
+#include <string.h>
+
+extern bool parse_argument (const char *, const char *);
+
+void
+parse_env (bool (* parse_argument)(const char *, const char *))
+{
+  char arg[2048];
+  const char * env;
+
+  if ((env = getenv ("ANNOBIN")) == NULL)
+    return;
+
+  while (*env != 0)
+    {
+      const char * comma;
+
+      comma = strchr (env, ',');
+      if (comma)
+	{
+	  strncpy (arg, env, comma - env);
+	  arg[comma - env] = 0;
+	  env = comma + 1;
+        }
+      else
+	{
+	  strncpy (arg, env, sizeof arg - 1);
+	  arg[sizeof arg - 1] = 0;
+	  env += strlen (env);
+	}
+
+      char * value = strchr (arg, '=');
+      if (value)
+	{
+	  * value = 0;
+	  value ++;
+	}
+      else
+	{
+	  value = (char *) "";
+	}
+
+      (void) (*parse_argument) (arg, value);
+    }
+}
diff --git a/annobin-common.h b/annobin-common.h
new file mode 100644
index 0000000..8007c6c
--- /dev/null
+++ b/annobin-common.h
@@ -0,0 +1,19 @@
+/* annobin - Common functions used by plugins.
+   Copyright (c) 2024 Red Hat.
+
+  This is free software; you can redistribute it and/or modify it
+  under the terms of the GNU General Public License as published
+  by the Free Software Foundation; either version 3, or (at your
+  option) any later version.
+
+  It is distributed in the hope that it will be useful, but
+  WITHOUT ANY WARRANTY; without even the implied warranty of
+  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+  GNU General Public License for more details.  */
+#ifndef ANNOBIN_COMMON_H_
+#define ANNOBIN_COMMON_H_
+
+#include <stdbool.h>
+
+void parse_env (bool (* parse_argument)(const char *, const char *));
+#endif // ANNOBIN-COMMON_H_
diff --git a/clang-plugin/Makefile.in b/clang-plugin/Makefile.in
index 1fc45c0..9ceba39 100644
--- a/clang-plugin/Makefile.in
+++ b/clang-plugin/Makefile.in
@@ -65,7 +65,9 @@ PLUGIN_TEST_OPTIONS = \
 #   -flto            # Not used because the object file generated is not an ELF format file.
 #   -fcf-protection  # Not used because not supported by all architectures
 
-check: @srcdir@/hello.c $(PLUGIN_NAME)
+TESTS = test-global-file-syms
+
+check: @srcdir@/hello.c $(PLUGIN_NAME) $(addsuffix .log,$(TESTS))
 	$(CLANG) -fplugin=$(PLUGIN) $(PLUGIN_TEST_OPTIONS) -c @srcdir@/hello.c
 	$(READELF) --wide --notes hello.o > clang-plugin-test.readelf.out
 	@ grep --silent -e "annobin built by clang version" clang-plugin-test.readelf.out
@@ -74,3 +76,11 @@ check: @srcdir@/hello.c $(PLUGIN_NAME)
 	@ grep --silent -e "SpecLoadHarden" clang-plugin-test.readelf.out
 	$(ANNOCHECK) --skip-all --test-optimization --test-fortify --test-stack-prot  hello.o --verbose
 	@ echo "PASS Clang plugin test"
+
+test-global-file-syms.log: @srcdir@/hello.c $(PLUGIN_NAME)
+	$(CLANG) -fplugin=$(PLUGIN) -fplugin-arg-annobin-global-file-syms -c $< -o test-global-file-syms.o
+	$(READELF) --wide --syms  test-global-file-syms.o > test-global-file-syms.readelf.out
+	@ grep --silent -e "_annobin.\+_hello_c_" test-global-file-syms.readelf.out
+	@ grep --silent -e '_annobin.\+_hello_c_.\+_start' test-global-file-syms.readelf.out
+	@ grep --silent -e '_annobin.\+_hello_c_.\+_end' test-global-file-syms.readelf.out
+	@ echo "PASS Clang global-file-syms test" | tee $@
diff --git a/clang-plugin/annobin.cpp b/clang-plugin/annobin.cpp
index 5426d22..6e1a8d4 100644
--- a/clang-plugin/annobin.cpp
+++ b/clang-plugin/annobin.cpp
@@ -31,12 +31,19 @@ using namespace llvm;
 #include <cstring>
 #include <cctype>
 #include <cstdarg>
+#include <iomanip>
 #include <sstream>
+#include <sys/time.h>
 
 namespace
 {
   static const unsigned int   annobin_version = (unsigned int) (ANNOBIN_VERSION * 100);
   bool                        be_verbose = false;
+  /* True if the symbols used to map addresses to file names should be global.
+     On some architectures these symbols have to be global so that they will
+     be preserved in object files.  But doing so can prevent the build-id
+     mechanism from working, since the symbols contain build-date information.  */
+  bool                        global_file_name_symbols = false;
 
   // Helper functions used throughout this file.
   template<class... Tys>
@@ -188,6 +195,30 @@ private:
       for( auto & c : name)
 	if (!isalnum (c))
 	  c = '_';
+
+      if (global_file_name_symbols)
+	{
+	  /* A program can have multiple source files with the same name.
+	     Or indeed the same source file can be included multiple times.
+	     Or a library can be built from a sources which include file names
+	     that match application file names.  Whatever the reason, we need
+	     to be ensure that we generate unique global symbol names.  So we
+	     append the time to the symbol name.  This will of course break
+	     the functionality of build-ids.  That is why this option is off
+	     by default.  */
+	  struct timeval tv;
+
+	  if (gettimeofday (& tv, NULL))
+	    {
+	      ice ("unable to get time of day.");
+	      tv.tv_sec = tv.tv_usec = 0;
+	    }
+	  std::ostringstream t;
+	  t << "_" << std::setfill('0') << std::setw(8) << (long) tv.tv_sec;
+	  t << "_" << std::setfill('0') << std::setw(8) << (long) tv.tv_usec;
+	  name += t.str();
+    }
+
     }
     
     void
@@ -574,11 +605,13 @@ private:
 	{
 	  if (args[i] == "help")
 	    inform ("supported options:\n\
-  help      Display this message\n\
-  disable   Disable the plugin\n\
-  enable    Reenable the plugin if it has been disabled\n\
-  version   Displays the version number\n\
-  verbose   Produce descriptive messages whilst working");
+  help               Display this message\n\
+  disable            Disable the plugin\n\
+  enable             Reenable the plugin if it has been disabled\n\
+  version            Displays the version number\n\
+  verbose            Produce descriptive messages whilst working\n\
+  \n\
+  global-file-syms   Create final name symbols that are global");
 	  else if (args[i] == "disable")
 	    enabled = false;
 	  else if (args[i] == "enable")
@@ -587,6 +620,8 @@ private:
 	    inform ("Annobin plugin version: %u", annobin_version);
 	  else if (args[i] == "verbose")
 	    be_verbose = true;
+	  else if (args[i] == "global-file-syms")
+	    global_file_name_symbols = true;
 	  else
 	    inform ("error: unknown option: %s", args[i].c_str());
 	}
diff --git a/gcc-plugin/Makefile.am b/gcc-plugin/Makefile.am
index eca5e10..9616018 100644
--- a/gcc-plugin/Makefile.am
+++ b/gcc-plugin/Makefile.am
@@ -8,7 +8,7 @@ plugin_LTLIBRARIES = annobin.la
 AM_CPPFLAGS = -I'$(top_builddir)' -I'$(top_srcdir)'
 AUTOMAKE_OPTIONS = no-dependencies
 
-annobin_la_SOURCES = annobin.cc
+annobin_la_SOURCES = annobin.cc ../annobin-common.cc
 EXTRA_annobin_la_SOURCES = aarch64.annobin.cc arm.annobin.cc dummy.annobin.cc powerpc.annobin.cc s390.annobin.cc x86_64.annobin.cc i686.annobin.cc riscv.annobin.cc mips.annobin.cc
 annobin_la_LIBADD = @target_plugin@
 annobin_la_DEPENDENCIES = @target_plugin@
diff --git a/gcc-plugin/Makefile.in b/gcc-plugin/Makefile.in
index d5604cc..6ecc51f 100644
--- a/gcc-plugin/Makefile.in
+++ b/gcc-plugin/Makefile.in
@@ -144,7 +144,7 @@ am__uninstall_files_from_dir = { \
   }
 am__installdirs = "$(DESTDIR)$(plugindir)"
 LTLIBRARIES = $(plugin_LTLIBRARIES)
-am_annobin_la_OBJECTS = annobin.lo
+am_annobin_la_OBJECTS = annobin.lo annobin-common.lo
 annobin_la_OBJECTS = $(am_annobin_la_OBJECTS)
 AM_V_P = $(am__v_P_@AM_V@)
 am__v_P_ = $(am__v_P_@AM_DEFAULT_V@)
@@ -347,7 +347,7 @@ top_srcdir = @top_srcdir@
 plugin_LTLIBRARIES = annobin.la
 AM_CPPFLAGS = -I'$(top_builddir)' -I'$(top_srcdir)'
 AUTOMAKE_OPTIONS = no-dependencies
-annobin_la_SOURCES = annobin.cc
+annobin_la_SOURCES = annobin.cc ../annobin-common.cc
 EXTRA_annobin_la_SOURCES = aarch64.annobin.cc arm.annobin.cc dummy.annobin.cc powerpc.annobin.cc s390.annobin.cc x86_64.annobin.cc i686.annobin.cc riscv.annobin.cc mips.annobin.cc
 annobin_la_LIBADD = @target_plugin@
 annobin_la_DEPENDENCIES = @target_plugin@
@@ -460,6 +460,9 @@ distclean-compile:
 .cc.lo:
 	$(AM_V_CXX)$(LTCXXCOMPILE) -c -o $@ $<
 
+annobin-common.lo: ../annobin-common.cc
+	$(AM_V_CXX)$(LIBTOOL) $(AM_V_lt) --tag=CXX $(AM_LIBTOOLFLAGS) $(LIBTOOLFLAGS) --mode=compile $(CXX) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) $(AM_CXXFLAGS) $(CXXFLAGS) -c -o annobin-common.lo `test -f '../annobin-common.cc' || echo '$(srcdir)/'`../annobin-common.cc
+
 mostlyclean-libtool:
 	-rm -f *.lo
 
diff --git a/gcc-plugin/annobin.cc b/gcc-plugin/annobin.cc
index 440029e..af1e2dd 100644
--- a/gcc-plugin/annobin.cc
+++ b/gcc-plugin/annobin.cc
@@ -16,6 +16,7 @@
 #include <stdio.h>
 #include <intl.h>
 
+#include "annobin-common.h"
 #include "annobin-global.h"
 #include "annobin.h"
 
@@ -3123,43 +3124,6 @@ parse_args (unsigned argc, struct plugin_argument * argv)
   return result;
 }
 
-static void
-parse_env (const char * env)
-{
-  while (*env != 0)
-    {
-      const char * comma;
-      char * arg = annobin_note_buffer;
-
-      comma = strchr (env, ',');
-      if (comma)
-	{
-	  strncpy (arg, env, comma - env);
-	  arg[comma - env] = 0;
-	  env = comma + 1;
-	}
-      else
-	{
-	  strncpy (arg, env, sizeof annobin_note_buffer - 1);
-	  arg[sizeof annobin_note_buffer - 1] = 0;
-	  env += strlen (env);
-	}
-
-      char * value = strchr (arg, '=');
-      if (value)
-	{
-	  * value = 0;
-	  value ++;
-	}
-      else
-	{
-	  value = (char *) "";
-	}
-
-      (void) parse_argument (arg, value);
-    }
-}
-
 int
 plugin_init (struct plugin_name_args *    plugin_info,
              struct plugin_gcc_version *  version)
@@ -3173,12 +3137,7 @@ plugin_init (struct plugin_name_args *    plugin_info,
       return 1;
     }
 
-  const char * env;
-  if ((env = getenv ("ANNOBIN")) != NULL)
-    {
-      parse_env (env);
-    }
-
+  parse_env (&parse_argument);
   if (plugins_active_p () && (annobin_extra_prefix[0] == 0))
     {
       /* See BZ 2162746 for an example of why this is needed.  */
diff --git a/llvm-plugin/Makefile.in b/llvm-plugin/Makefile.in
index bcee371..f9b4e26 100644
--- a/llvm-plugin/Makefile.in
+++ b/llvm-plugin/Makefile.in
@@ -28,10 +28,15 @@ PLUGIN_NAME = annobin-for-llvm.so
 
 all: $(PLUGIN_NAME) Makefile
 
-$(PLUGIN_NAME): annobin.cpp
+$(PLUGIN_NAME): annobin.cpp annobin-common.o
 	clang++ $(CLANG_TARGET_OPTIONS) $(LLVM_CXX_OPTIONS) \
 		$(PLUGIN_OPTIONS) $(LLVM_LD_OPTIONS) $(LLVM_SYS_LIBS) \
-		-I .. -I$(INCDIR) $< -o $@
+		-I .. -I$(INCDIR) $^ -o $@
+
+annobin-common.o: @top_srcdir@/annobin-common.cc
+	clang++ -c $(CLANG_TARGET_OPTIONS) $(LLVM_CXX_OPTIONS) \
+		$(PLUGIN_OPTIONS) $(LLVM_LD_OPTIONS) $(LLVM_SYS_LIBS) \
+		-I .. -I$(INCDIR) $^ -o $@
 
 install: $(PLUGIN_NAME)
 	install -Dpm0755 -t ${PLUGIN_INSTALL_DIR} $<
@@ -63,6 +68,8 @@ PLUGIN_TEST_OPTIONS = \
 #   -flto            # Not used because the object file generated is not an ELF format file.
 #   -fcf-protection  # Not used because not supported by all architectures
 
+TESTS = test-global-file-syms
+
 check: $(PLUGIN_NAME)
 	@ if [ `echo | clang -dM -E - | grep __clang_major__ | cut -f 3 -d ' '` -gt 14 ] ; \
 	then \
@@ -85,7 +92,7 @@ check-newpm:
 check-pre-clang-13:
 	$(MAKE) check-run LOAD_PLUGIN_ARG="-Xclang -load -Xclang $(PLUGIN)"
 
-check-run: @srcdir@/hello.c
+check-run: @srcdir@/hello.c $(addsuffix .log,$(TESTS))
 	echo Compiling with $(LOAD_PLUGIN_ARG) ...
 	$(CLANG) $(LOAD_PLUGIN_ARG)  $(PLUGIN_TEST_OPTIONS) -c @srcdir@/hello.c
 	echo Checking with readelf ...
@@ -97,3 +104,11 @@ check-run: @srcdir@/hello.c
 	$(ANNOCHECK) --skip-all --test-optimization --test-fortify --test-stack-prot hello.o || :
 	$(ANNOCHECK) --skip-all --test-optimization --test-fortify --test-stack-prot hello.o
 	echo "PASS LLVM plugin test [$(LOAD_PLUGIN_ARG)]"
+
+test-global-file-syms.log: @srcdir@/hello.c
+	ANNOBIN=global-file-syms $(CLANG) $(LOAD_PLUGIN_ARG) -c $< -o test-global-file-syms.o
+	$(READELF) --wide --syms  test-global-file-syms.o > test-global-file-syms.readelf.out
+	@ grep --silent -e "_annobin.\+_hello_c_" test-global-file-syms.readelf.out
+	@ grep --silent -e '_annobin.\+_hello_c_.\+_start' test-global-file-syms.readelf.out
+	@ grep --silent -e '_annobin.\+_hello_c_.\+_end' test-global-file-syms.readelf.out
+	@ echo "PASS LLVM global-file-syms test" | tee $@
diff --git a/llvm-plugin/annobin.cpp b/llvm-plugin/annobin.cpp
index fe3e036..08b6241 100644
--- a/llvm-plugin/annobin.cpp
+++ b/llvm-plugin/annobin.cpp
@@ -25,16 +25,24 @@
 #endif
 #include "llvm/LTO/legacy/LTOCodeGenerator.h"
 #include "annobin-global.h"
+#include "annobin-common.h"
 #include <cstring>
 #include <cctype>
 #include <cstdarg>
+#include <iomanip>
 #include <sstream>
+#include <sys/time.h>
 
 using namespace llvm;
 namespace
 {
   static bool                 be_verbose = false;
   static unsigned int         target_start_sym_bias = 0;
+  /* True if the symbols used to map addresses to file names should be global.
+     On some architectures these symbols have to be global so that they will
+     be preserved in object files.  But doing so can prevent the build-id
+     mechanism from working, since the symbols contain build-date information.  */
+  bool                        global_file_name_symbols = false;
 
   // Helper functions used throughout this file.
   template<class... Tys>
@@ -46,7 +54,6 @@ namespace
     (void) std::initializer_list<int>{((oss << args), 1)...};
     return strdup (oss.str().c_str());
   }
-#if 0
   static inline void
   inform (char const fmt[], ...)
   {
@@ -59,7 +66,6 @@ namespace
     fputc ('\n', stderr);
     va_end (args);
   }
-#endif
   static inline void
   verbose (char const fmt[], ...)
   {
@@ -278,6 +284,24 @@ namespace
       free (buffer);
     }
 
+    static bool
+    parse_argument (const char * key, const char * value)
+    {
+      if (streq (key, "verbose"))
+	be_verbose = true;
+      else if (streq (key, "global-file-syms"))
+	global_file_name_symbols = true;
+      else if (streq (key, "no-global-file-syms"))
+	global_file_name_symbols = false;
+      else
+	{
+	  inform ("annobin: unrecognised option: %s\n", key);
+	  return false;
+        }
+
+      return true;
+    }
+
   public:
 #if __clang_major__ > 12
     AnnobinModule()
@@ -289,6 +313,8 @@ namespace
       if (getenv ("ANNOBIN_VERBOSE") != NULL
 	  && ! streq (getenv ("ANNOBIN_VERBOSE"), "false"))
 	be_verbose = true;
+
+      parse_env (&parse_argument);
     }
 
     void
@@ -486,6 +512,29 @@ namespace
       for( auto & c : name)
 	if (!isalnum (c))
 	  c = '_';
+
+      if (global_file_name_symbols)
+	{
+	  /* A program can have multiple source files with the same name.
+	     Or indeed the same source file can be included multiple times.
+	     Or a library can be built from a sources which include file names
+	     that match application file names.  Whatever the reason, we need
+	     to be ensure that we generate unique global symbol names.  So we
+	     append the time to the symbol name.  This will of course break
+	     the functionality of build-ids.  That is why this option is off
+	     by default.  */
+	  struct timeval tv;
+
+	  if (gettimeofday (& tv, NULL))
+	    {
+	      ice ("unable to get time of day.");
+	      tv.tv_sec = tv.tv_usec = 0;
+	    }
+	  std::ostringstream t;
+	  t << "_" << std::setfill('0') << std::setw(8) << (long) tv.tv_sec;
+	  t << "_" << std::setfill('0') << std::setw(8) << (long) tv.tv_usec;
+	  name += t.str();
+	}
     }
 
     static void
-- 
2.44.0

Re: [PATCH] Add support for global-file-syms to the clang and llvm plugins

[Nick Clifton]

Hi Tulio,

> This part starts by moving the implementation of parse_env() to
> annobin-common.cc in order to be shared between the gcc and the llvm
> plugins.
> 
> This was required because the llvm plugin didn't have a mechanism to
> receive arguments from the user. This commit adds support for using the
> environment variable ANNOBIN for the LLVM plugin, although it's still
> missing many of the basic features available in the other plugins.
> 
> Both clang and llvm plugins are now able to generate the same output as
> the GCC plugin, e.g. _annobin_hello_c_1711138217_00204218_start.

Thanks very much for writing this patch.

I do have a few comments on the code, but overall I thought that it was
excellent.

> diff --git a/annobin-common.cc b/annobin-common.cc

> +extern bool parse_argument (const char *, const char *);

There is no external function called parse_argument() so you do not
have to provide a prototype for it...

> +void
> +parse_env (bool (* parse_argument)(const char *, const char *))

I think that the function should be called 'annobin_parse_env' rather
than 'parse_env'.  There are two reasons: 1) there is a distinct
possibility that it might clash with a same-named function in the
sources of Clang, LLVM or GCC one day.  2) when running the compiler
inside a debugger it is helpful to have function names that indicate
to which part of the program they belong.  Thus a user using gdb to
debug a clang crash for example might run across "parse_env" and have
no idea where to look for it in the sources.  But if they see
"annobin_parse_env" they immediately have a big clue for where to look.

Also - the function should return a BOOL based upon the results
of calling parse_argument().  Ie if any invocation of parse_argument()
fails, then parse_env() should also fail.

Also - the parse_argument function should take a third argument, a void *
pointer provided to parse_env() by the caller.  The reason for this is
that the caller may wish to pass extra information to the parse_argument
function.


> +{
> +  char arg[2048];

Large buffers on the stack are a security hazzard.  Better to make
the buffer static - or better yet, dynamic.

> +      comma = strchr (env, ',');
> +      if (comma)
> +	{
> +	  strncpy (arg, env, comma - env);

Potential buffer overflow if (common - env >= sizeof arg)


> diff --git a/annobin-common.h b/annobin-common.h

> +void parse_env (bool (* parse_argument)(const char *, const char *));
> +#endif // ANNOBIN-COMMON_H_

I am a firm believer in header files providing descriptions of the functions
that they prototype.  This makes it easier for users unfamiliar with the source
code to use the header without having to delve into the sources themselves.
So in the version of your patch that I have checked in I added:

/* Checks for an ANNOBIN environment variable.
    If it exists, parses it for comma separated command line options,
     passing each in turn to PARSE_ARGUMENT.  Arguments are copied
     into a buffer before processing and can be manipulated by
     PARSE_ARGUMENT.  If an argument contains a '=' character then
     it is split into two pieces and passed to PARSE_ARGUMENT as
     (NAME, VALUE).  Otherwise it is just passed as (NAME, "").
     The return value from PARSE_ARGUMENT is recorded, but
     parsing will continue even if PARSE_ARGUMENT returns FALSE.
    Returns TRUE if the ANNOBIN environment variable does not exist.
    Returns TRUE if the ANNOBIN environment exists and PARSE_ARGUMENT
     returns TRUE for all of the arguments.
    Return FALSE otherwise.  */


I also thought that we should be consistent and allow the clang plugin to also
parse the ANNOBIN environment variable.  So I added that.  But basically I
was very happy with your patch and I have now checked it in to the annobin
repository.

Cheers
   Nick

PS. I have not built the rawhide version yet as I have run into some problems with the testsuites...

Re: global-file-syms for clang

[Nick Clifton]

Hi Grant,

> Thanks. As a data point, when I use the GCC toolchain, I didn't need to set the global-file-syms flag. I believe the issue is an interaction between the LLVM gold plugin 
> and the annobin plugin and linking in LTO archives that have objects that have the same filename for the source. The LLVM gold plugin tries to bring in all the fine annobin 
> symbols, rather than just bringing the annobin symbols from the first file that satisfies the symbols for linking.

Hmm, strange.  It sounds like an issue with the LLVM plugin, but I doubt
if it is likely to be fixed...

> I was just going to use the global-file-syms to overcome the LLVM gold plugin/LTO issue.

Fair enough.  Please try the 12.46 sources, available from the annobin repository.
This has Tulio's patch to implement the global-file-syms option for the Clang and
LLVM plugins.  (For the LLVM plugin you have to pass the options in the ANNOBIN
environment variable, since LLVM does not support passing options to plugins).

Cheers
   Nick

RE: global-file-syms for clang

[grant]

Nick,

Thanks! Couple of questions:
1) Will the new annobin plugin/annocheck work with clang version16.0.6-1.el9)? Specifically, will the interaction between annocheck & the compile cause any issues/false positives or negatives?
2) I noticed that annocheck has a considerable number of errors around gaps. This is after regenerating all the source (except standard libraries) with annobin plugins. Is this expected. For reference, LTO is enabled in these cases.

Thanks again,
Grant Haidinyak

-----Original Message-----
From: Nick Clifton <nickc@redhat.com> 
Sent: Monday, March 25, 2024 10:19 AM
To: grant@hcubed.com
Cc: annobin@sourceware.org; Tulio Magno Quites Machado Filho <tuliom@redhat.com>
Subject: Re: global-file-syms for clang

Hi Grant,

> Thanks. As a data point, when I use the GCC toolchain, I didn't need 
> to set the global-file-syms flag. I believe the issue is an 
> interaction between the LLVM gold plugin and the annobin plugin and linking in LTO archives that have objects that have the same filename for the source. The LLVM gold plugin tries to bring in all the fine annobin symbols, rather than just bringing the annobin symbols from the first file that satisfies the symbols for linking.

Hmm, strange.  It sounds like an issue with the LLVM plugin, but I doubt if it is likely to be fixed...

> I was just going to use the global-file-syms to overcome the LLVM gold plugin/LTO issue.

Fair enough.  Please try the 12.46 sources, available from the annobin repository.
This has Tulio's patch to implement the global-file-syms option for the Clang and LLVM plugins.  (For the LLVM plugin you have to pass the options in the ANNOBIN environment variable, since LLVM does not support passing options to plugins).

Cheers
   Nick

Re: global-file-syms for clang

[Nick Clifton]

Hi Grant,

> 1) Will the new annobin plugin/annocheck work with clang version16.0.6-1.el9)?

Yes.  Well assuming that you are using a version of the annobin clang
plugin that was compiled with clang 16.

If you are using RHEL-9 then there may be a slight issue in that annobin
version 12.46 has not been made available for CentOS-9/RHEL-9 yet.  You
are always free to compile it yourself, but if you want to use the official
CentOS-9/RHEL-9 annobin rpm then you will probably need to file a bug
report/enhancement request to get the new version.

As for annocheck, it is completely architecture and version agnostic.
It will run on any architecture and can check files built for any
architecture (not just the architecture type of the host on which it
is run) by any version of any compiler.


> Specifically, will the interaction between annocheck & the compile cause any issues/false positives or negatives?

No.  Or at least it should not, and if any issues or false positives
do arise then this is due to bug(s) in annocheck.  (In which case
please do report them...)


> 2) I noticed that annocheck has a considerable number of errors around gaps. This is after regenerating all the source (except standard libraries) with annobin plugins. Is this expected. For reference, LTO is enabled in these cases.

Err, maybe ?

It depends upon where the gaps are located.  If they are in those
standard libraries that have not been recompiled, then it would
make sense.  If the gaps are in code that has been recompiled and
therefore should have annobin data, then something has gone wrong.

LTO compilation does cause problems for annobin and annocheck, but
these days they should mostly be solved.  Having said that though,
I have mostly been working with LTO using GCC rather than Clang,
so there could still be some issues that need addressing.

If you have an example binary that you would care to upload I can
take a closer look and maybe find out some more.

Cheers
   Nick