[PATCH] ld: Add support for a new option, -exclude-symbols, in COFF object file directives

[PATCH] ld: Add support for a new option, -exclude-symbols, in COFF object file directives

[Martin Storsjo]

This maps to the same as ld's --exclude-symbols command line option,
but allowing specifying the option via directives embedded in the
object files instead of passed manually on the command line.
---
I've posted a corresponding patch to LLD in
https://reviews.llvm.org/D130120 - I'd like to have support for it
in both linkers before I'd take the option into use.
---
 ld/deffile.h  |  6 ++++++
 ld/deffilep.y | 43 ++++++++++++++++++++++++++++++++++++++++++-
 ld/pe-dll.c   | 10 ++++++++++
 3 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/ld/deffile.h b/ld/deffile.h
index d68fa8c6ba1..306ae3a6f75 100644
--- a/ld/deffile.h
+++ b/ld/deffile.h
@@ -61,6 +61,11 @@ typedef struct def_file_aligncomm {
   unsigned int alignment;	/* log-2 alignment.        */
 } def_file_aligncomm;
 
+typedef struct def_file_exclude_symbol {
+  struct def_file_exclude_symbol *next;	/* Chain pointer.  */
+  char *symbol_name;		/* Name of excluded symbol.  */
+} def_file_exclude_symbol;
+
 typedef struct def_file {
   /* From the NAME or LIBRARY command.  */
   char *name;
@@ -94,6 +99,7 @@ typedef struct def_file {
 
   /* Only expected from .drectve sections, not .DEF files.  */
   def_file_aligncomm *aligncomms;
+  def_file_exclude_symbol *exclude_symbols;
 
 } def_file;
 
diff --git a/ld/deffilep.y b/ld/deffilep.y
index 3f610ddbe14..a7db23eace6 100644
--- a/ld/deffilep.y
+++ b/ld/deffilep.y
@@ -101,6 +101,7 @@ static void def_stacksize (int, int);
 static void def_version (int, int);
 static void def_directive (char *);
 static void def_aligncomm (char *str, int align);
+static void def_exclude_symbols (char *str);
 static int def_parse (void);
 static void def_error (const char *);
 static int def_lex (void);
@@ -121,7 +122,7 @@ static const char *lex_parse_string_end = 0;
 
 %token NAME LIBRARY DESCRIPTION STACKSIZE_K HEAPSIZE CODE DATAU DATAL
 %token SECTIONS EXPORTS IMPORTS VERSIONK BASE CONSTANTU CONSTANTL
-%token PRIVATEU PRIVATEL ALIGNCOMM
+%token PRIVATEU PRIVATEL ALIGNCOMM EXCLUDE_SYMBOLS
 %token READ WRITE EXECUTE SHARED_K NONAMEU NONAMEL DIRECTIVE EQUAL
 %token <id> ID
 %token <digits> DIGITS
@@ -155,6 +156,7 @@ command:
 	|	VERSIONK NUMBER '.' NUMBER { def_version ($2, $4);}
 	|	DIRECTIVE ID { def_directive ($2);}
 	|	ALIGNCOMM anylang_id ',' NUMBER { def_aligncomm ($2, $4);}
+	|	EXCLUDE_SYMBOLS anylang_id { def_exclude_symbols ($2);}
 	;
 
 
@@ -488,6 +490,15 @@ def_file_free (def_file *fdef)
       free (c);
     }
 
+  while (fdef->exclude_symbols)
+    {
+      def_file_exclude_symbol *e = fdef->exclude_symbols;
+
+      fdef->exclude_symbols = fdef->exclude_symbols->next;
+      free (e->symbol_name);
+      free (e);
+    }
+
   free (fdef);
 }
 
@@ -946,6 +957,7 @@ diropts[] =
   { "-attr", SECTIONS },
   { "-export", EXPORTS },
   { "-aligncomm", ALIGNCOMM },
+  { "-exclude-symbols", EXCLUDE_SYMBOLS },
   { 0, 0 }
 };
 
@@ -1261,6 +1273,35 @@ def_aligncomm (char *str, int align)
     }
 }
 
+static void
+def_exclude_symbols (char *str)
+{
+  def_file_exclude_symbol *c, *p;
+
+  p = NULL;
+  c = def->exclude_symbols;
+  while (c != NULL)
+    {
+      int e = strcmp (c->symbol_name, str);
+      if (!e)
+        return;
+      c = (p = c)->next;
+    }
+
+  c = xmalloc (sizeof (def_file_exclude_symbol));
+  c->symbol_name = xstrdup (str);
+  if (!p)
+    {
+      c->next = def->exclude_symbols;
+      def->exclude_symbols = c;
+    }
+  else
+    {
+      c->next = p->next;
+      p->next = c;
+    }
+}
+
 static void
 def_error (const char *err)
 {
diff --git a/ld/pe-dll.c b/ld/pe-dll.c
index db2c75da9b5..33be53d6164 100644
--- a/ld/pe-dll.c
+++ b/ld/pe-dll.c
@@ -718,6 +718,16 @@ process_def_file_and_drectve (bfd *abfd ATTRIBUTE_UNUSED, struct bfd_link_info *
 	}
     }
 
+  if (pe_def_file->exclude_symbols)
+    {
+      def_file_exclude_symbol *ac = pe_def_file->exclude_symbols;
+      while (ac)
+	{
+	  pe_dll_add_excludes (ac->symbol_name, EXCLUDESYMS);
+	  ac = ac->next;
+	}
+    }
+
   /* If we are building an executable and there is nothing
      to export, we do not build an export table at all.  */
   if (bfd_link_executable (info) && pe_def_file->num_exports == 0
-- 
2.25.1

Re: [PATCH] ld: Add support for a new option, -exclude-symbols, in COFF object file directives

[Jan Beulich]

On 19.07.2022 22:33, Martin Storsjo wrote:
> @@ -946,6 +957,7 @@ diropts[] =
>    { "-attr", SECTIONS },
>    { "-export", EXPORTS },
>    { "-aligncomm", ALIGNCOMM },
> +  { "-exclude-symbols", EXCLUDE_SYMBOLS },

I was a little concerned by the - in the middle, but the (custom)
def-file lexer looks to be treating - the same as ISALPHA() chars.
Nevertheless it would be quite helpful if there were two testcases
here - one with the new directive used in an object file's .drectve
section and another with it used in a .def file.

The other thing is: According to def_exclude_symbols() each
directive is followed by exactly one symbol afaict. Hence
-exclude-symbol (singular) would seem more appropriate for the
option.

Jan

Re: [PATCH] ld: Add support for a new option, -exclude-symbols, in COFF object file directives

[Martin Storsjö]

On Wed, 20 Jul 2022, Jan Beulich wrote:

> On 19.07.2022 22:33, Martin Storsjo wrote:
>> @@ -946,6 +957,7 @@ diropts[] =
>>    { "-attr", SECTIONS },
>>    { "-export", EXPORTS },
>>    { "-aligncomm", ALIGNCOMM },
>> +  { "-exclude-symbols", EXCLUDE_SYMBOLS },

Thanks for having a look!

> I was a little concerned by the - in the middle, but the (custom)
> def-file lexer looks to be treating - the same as ISALPHA() chars.

Thanks for checking!

> Nevertheless it would be quite helpful if there were two testcases
> here - one with the new directive used in an object file's .drectve
> section and another with it used in a .def file.

A testcase certainly would be good here. I'm not familiar with binutils' 
tests from before - I presume I'd make a native build (not having binutils 
set up with a cross target) and run "make check", and I'd find examples of 
existing tests in ld/testsuite/ld-pe?

I guess it'd be good with one test for i386 and one for x86_64 (or both 
cases merged into one?), assembling an .s file and linking it, and then 
checking the export table. Are there any similar existing tests to serve 
as example?

This directive isn't supposed to be used from a def file though (just like 
the aligncomm directive), see the comment in deffile.h.

> The other thing is: According to def_exclude_symbols() each
> directive is followed by exactly one symbol afaict. Hence
> -exclude-symbol (singular) would seem more appropriate for the
> option.

Actually, when the option values later are added to the exclusion list by 
pe_dll_add_excludes, they're split/tokenized by commas (and colon, 
apparently). I guess this could be made clearer by renaming the struct 
field though, maybe to "symbol_names"?

I'd prefer to keep the plural form in the directive name in any case, for 
consistency with the command line option, even if it uses the 
-<name>:<value> option form (like all other embedded directives) instead 
of --<name>=<value> or --<name> <value> on the command line.

// Martin

Re: [PATCH] ld: Add support for a new option, -exclude-symbols, in COFF object file directives

[Jan Beulich]

On 20.07.2022 14:39, Martin Storsjö wrote:
> On Wed, 20 Jul 2022, Jan Beulich wrote:
>> On 19.07.2022 22:33, Martin Storsjo wrote:
>>> @@ -946,6 +957,7 @@ diropts[] =
>>>    { "-attr", SECTIONS },
>>>    { "-export", EXPORTS },
>>>    { "-aligncomm", ALIGNCOMM },
>>> +  { "-exclude-symbols", EXCLUDE_SYMBOLS },
> 
> Thanks for having a look!
> 
>> I was a little concerned by the - in the middle, but the (custom)
>> def-file lexer looks to be treating - the same as ISALPHA() chars.
> 
> Thanks for checking!
> 
>> Nevertheless it would be quite helpful if there were two testcases
>> here - one with the new directive used in an object file's .drectve
>> section and another with it used in a .def file.
> 
> A testcase certainly would be good here. I'm not familiar with binutils' 
> tests from before - I presume I'd make a native build (not having binutils 
> set up with a cross target) and run "make check", and I'd find examples of 
> existing tests in ld/testsuite/ld-pe?

Cross builds ought to be fine - you don't mean to run any resulting
executable anyway.

> I guess it'd be good with one test for i386 and one for x86_64 (or both 
> cases merged into one?), assembling an .s file and linking it, and then 
> checking the export table. Are there any similar existing tests to serve 
> as example?

The one I spotted was for aligncomm, in ld-pe. In how far this would
lend itself to cloning I can't easily tell.

> This directive isn't supposed to be used from a def file though (just like 
> the aligncomm directive), see the comment in deffile.h.

Isn't supposed to be, or is prevented from being used that way? I have
to admit that I'd find it quite helpful if this new option was also
usable from a .def file. This might commonly be the more central place
to record exclusions.

>> The other thing is: According to def_exclude_symbols() each
>> directive is followed by exactly one symbol afaict. Hence
>> -exclude-symbol (singular) would seem more appropriate for the
>> option.
> 
> Actually, when the option values later are added to the exclusion list by 
> pe_dll_add_excludes, they're split/tokenized by commas (and colon, 
> apparently).

Well, if that's the case (I simply didn't spot this tokenization then),
then plural ifs of course fine.

Jan

> I guess this could be made clearer by renaming the struct 
> field though, maybe to "symbol_names"?
> 
> I'd prefer to keep the plural form in the directive name in any case, for 
> consistency with the command line option, even if it uses the 
> -<name>:<value> option form (like all other embedded directives) instead 
> of --<name>=<value> or --<name> <value> on the command line.
> 
> // Martin
> 

Re: [PATCH] ld: Add support for a new option, -exclude-symbols, in COFF object file directives

[Martin Storsjö]

On Wed, 20 Jul 2022, Jan Beulich wrote:

> On 20.07.2022 14:39, Martin Storsjö wrote:
>> This directive isn't supposed to be used from a def file though (just like
>> the aligncomm directive), see the comment in deffile.h.
>
> Isn't supposed to be, or is prevented from being used that way? I have
> to admit that I'd find it quite helpful if this new option was also
> usable from a .def file. This might commonly be the more central place
> to record exclusions.

It's not possible to use it in a .def file with the current patch - it 
would need to be recognized as a token at the very least. Do you want me 
to try add support for that? I guess I could see a usecase for that too...

My intended usecase is to map __attribute__((visibility("hidden"))) and 
similar (-fvisibility=hidden and -fvisibility-inlines-hidden) to this 
embedded directive, in Clang/llvm. (Not sure how hard it would be to do 
the same in GCC, but I have a Clang patch ready for that.)

// Martin

Re: [PATCH] ld: Add support for a new option, -exclude-symbols, in COFF object file directives

[Jan Beulich]

On 20.07.2022 17:10, Martin Storsjö wrote:
> On Wed, 20 Jul 2022, Jan Beulich wrote:
> 
>> On 20.07.2022 14:39, Martin Storsjö wrote:
>>> This directive isn't supposed to be used from a def file though (just like
>>> the aligncomm directive), see the comment in deffile.h.
>>
>> Isn't supposed to be, or is prevented from being used that way? I have
>> to admit that I'd find it quite helpful if this new option was also
>> usable from a .def file. This might commonly be the more central place
>> to record exclusions.
> 
> It's not possible to use it in a .def file with the current patch - it 
> would need to be recognized as a token at the very least. Do you want me 
> to try add support for that? I guess I could see a usecase for that too...

Would be nice, but I'm not going to insist.

> My intended usecase is to map __attribute__((visibility("hidden"))) and 
> similar (-fvisibility=hidden and -fvisibility-inlines-hidden) to this 
> embedded directive, in Clang/llvm. (Not sure how hard it would be to do 
> the same in GCC, but I have a Clang patch ready for that.)

Ah, I see - thanks for sharing the context.

Jan