gold: Support percent-encoded JSON in --package-metadata

Message ID 20241127102459.1085557-1-benjamin.drung@canonical.com
State New
Headers
Series gold: Support percent-encoded JSON in --package-metadata |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed

Commit Message

Benjamin Drung Nov. 27, 2024, 10:24 a.m. UTC
  Specifying the compiler flag `-Wl,--package-metadata=<JSON>` will not
work in case the JSON contains a comma, because compiler drivers eat
commas. Example:

```
$ echo "void main() { }" > test.c
$ gcc -fuse-ld=gold '-Wl,--package-metadata={"type":"deb","os":"ubuntu"}' test.c
/usr/bin/ld.gold: error: cannot open "os":"ubuntu"}: No such file or directory
/usr/bin/ld.gold: fatal error: error: --package-metadata={"type":"deb" does not contain valid JSON: '}' expected near end of file

collect2: error: ld returned 1 exit status
```

The quotation marks in the JSON value do not work well with shell nor
make. Specifying the `--package-metadata` linker flag in a `LDFLAGS`
environment variable might loose its quotation marks when it hits the
final compiler call.

Following the same format as the implementation in ld:
b0cc81e87087bb8a6b12dc1e4fd7f2591927977b

So support percent-encoded and %[string] encoded JSON data in the
`--package-metadata` linker flag. Percent-encoding is used because it is
a standard, simple to implement, and does take too many additional
characters. %[string] encoding is supported for having a more readable
encoding.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32003
Bug-Ubutru: https://bugs.launchpad.net/bugs/2071468
Signed-off-by: Benjamin Drung <benjamin.drung@canonical.com>
---
 gold/options.cc            | 72 +++++++++++++++++++++++++++++++++
 gold/options.h             | 17 +++++++-
 gold/testsuite/Makefile.am | 16 +++++++-
 gold/testsuite/Makefile.in | 82 +++++++++++++++++++++++++++++++++-----
 4 files changed, 174 insertions(+), 13 deletions(-)
  

Comments

Benjamin Drung Dec. 17, 2024, 12:09 p.m. UTC | #1
Friendly ping since I haven't got a response since submission three
weeks ago.

On Wed, 2024-11-27 at 10:24 +0000, Benjamin Drung wrote:
> Specifying the compiler flag `-Wl,--package-metadata=<JSON>` will not
> work in case the JSON contains a comma, because compiler drivers eat
> commas. Example:
> 
> ```
> $ echo "void main() { }" > test.c
> $ gcc -fuse-ld=gold '-Wl,--package-metadata={"type":"deb","os":"ubuntu"}' test.c
> /usr/bin/ld.gold: error: cannot open "os":"ubuntu"}: No such file or directory
> /usr/bin/ld.gold: fatal error: error: --package-metadata={"type":"deb" does not contain valid JSON: '}' expected near end of file
> 
> collect2: error: ld returned 1 exit status
> ```
> 
> The quotation marks in the JSON value do not work well with shell nor
> make. Specifying the `--package-metadata` linker flag in a `LDFLAGS`
> environment variable might loose its quotation marks when it hits the
> final compiler call.
> 
> Following the same format as the implementation in ld:
> b0cc81e87087bb8a6b12dc1e4fd7f2591927977b
> 
> So support percent-encoded and %[string] encoded JSON data in the
> `--package-metadata` linker flag. Percent-encoding is used because it is
> a standard, simple to implement, and does take too many additional
> characters. %[string] encoding is supported for having a more readable
> encoding.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32003
> Bug-Ubutru: https://bugs.launchpad.net/bugs/2071468
> Signed-off-by: Benjamin Drung <benjamin.drung@canonical.com>
> ---
>  gold/options.cc            | 72 +++++++++++++++++++++++++++++++++
>  gold/options.h             | 17 +++++++-
>  gold/testsuite/Makefile.am | 16 +++++++-
>  gold/testsuite/Makefile.in | 82 +++++++++++++++++++++++++++++++++-----
>  4 files changed, 174 insertions(+), 13 deletions(-)
> 
> diff --git a/gold/options.cc b/gold/options.cc
> index 46f067fa72f..4f615d44e28 100644
> --- a/gold/options.cc
> +++ b/gold/options.cc
> @@ -255,6 +255,78 @@ parse_string(const char* option_name, const char* arg, const char** retval)
>    *retval = arg;
>  }
>  
> +// Decode a percent and/or %[string] encoded string. Following %[string]
> +// encodings are supported:
> +//
> +// %[comma] for ,
> +// %[lbrace] for {
> +// %[quot] for "
> +// %[rbrace] for }
> +// %[space] for ' '
> +//
> +// The percent decoding behaves the same as Python's urllib.parse.unquote.
> +void
> +parse_optional_encoded_string(const char* option_name, const char* arg, const char** retval)
> +{
> +  hex_init();
> +  size_t arg_len = strlen (arg);
> +  char *package_metadata = (char *)malloc (arg_len + 1);
> +  if (package_metadata == NULL)
> +    gold_fatal(_("%s: malloc failed"), option_name);
> +
> +  const char *src = arg;
> +  char *dst = package_metadata;
> +  while (*src != '\0')
> +    {
> +      char c = *src++;
> +      if (c == '%')
> +	{
> +	  char next1 = *src;
> +	  if (next1 != '\0' && hex_p(next1))
> +	    {
> +	      char next2 = *(src + 1);
> +	      if (next2 != '\0' && hex_p(next2))
> +		{
> +		  c = (char)((hex_value(next1) << 4) + hex_value(next2));
> +		  src += 2;
> +		}
> +	    }
> +	  else if (next1 == '[')
> +	    {
> +	      if (strncmp(src + 1, "comma]", 6) == 0)
> +		{
> +		  c = ',';
> +		  src += 7;
> +		}
> +	      else if (strncmp(src + 1, "lbrace]", 7) == 0)
> +		{
> +		  c = '{';
> +		  src += 8;
> +		}
> +	      else if (strncmp(src + 1, "quot]", 5) == 0)
> +		{
> +		  c = '"';
> +		  src += 6;
> +		}
> +	      else if (strncmp(src + 1, "rbrace]", 7) == 0)
> +		{
> +		  c = '}';
> +		  src += 8;
> +		}
> +	      else if (strncmp(src + 1, "space]", 6) == 0)
> +		{
> +		  c = ' ';
> +		  src += 7;
> +		}
> +	    }
> +	}
> +      *dst++ = c;
> +    }
> +  *dst = '\0';
> +
> +  *retval = package_metadata;
> +}
> +
>  void
>  parse_optional_string(const char*, const char* arg, const char** retval)
>  {
> diff --git a/gold/options.h b/gold/options.h
> index 446e8d42614..78766c1fc6b 100644
> --- a/gold/options.h
> +++ b/gold/options.h
> @@ -108,6 +108,10 @@ parse_percent(const char* option_name, const char* arg, double* retval);
>  extern void
>  parse_string(const char* option_name, const char* arg, const char** retval);
>  
> +extern void
> +parse_optional_encoded_string(const char* option_name, const char* arg,
> +			      const char** retval);
> +
>  extern void
>  parse_optional_string(const char* option_name, const char* arg,
>  		      const char** retval);
> @@ -589,6 +593,17 @@ struct Struct_special : public Struct_var
>    };                                                                    \
>    Struct_##varname__ varname__##_initializer_
>  
> +// An option that takes an optional string argument.  If the option is
> +// used with no argument, the value will be the default, and
> +// user_set_via_option will be true.
> +#define DEFINE_optional_encoded_string(varname__, dashes__,		\
> +				       shortname__, default_value__,	\
> +				       helpstring__, helparg__)		\
> +  DEFINE_var(varname__, dashes__, shortname__, default_value__,		\
> +	     default_value__, helpstring__, helparg__, true,		\
> +	     const char*, const char*,					\
> +	     options::parse_optional_encoded_string, false)
> +
>  // An option that takes an optional string argument.  If the option is
>  // used with no argument, the value will be the default, and
>  // user_set_via_option will be true.
> @@ -1106,7 +1121,7 @@ class General_options
>    DEFINE_bool(p, options::ONE_DASH, 'p', false,
>  	      N_("Ignored for ARM compatibility"), NULL);
>  
> -  DEFINE_optional_string(package_metadata, options::TWO_DASHES, '\0', NULL,
> +  DEFINE_optional_encoded_string(package_metadata, options::TWO_DASHES, '\0', NULL,
>  			 N_("Generate package metadata note"),
>  			 N_("[=JSON]"));
>  
> diff --git a/gold/testsuite/Makefile.am b/gold/testsuite/Makefile.am
> index 8f158ba20cc..d7f496c75c1 100644
> --- a/gold/testsuite/Makefile.am
> +++ b/gold/testsuite/Makefile.am
> @@ -4470,9 +4470,21 @@ retain_2.o: retain_2.s
>  
>  endif DEFAULT_TARGET_X86_64
>  
> -check_PROGRAMS += package_metadata_test
> +check_PROGRAMS += package_metadata_test1 package_metadata_test1b package_metadata_test1c package_metadata_test2 package_metadata_test2b
>  package_metadata_test.o: package_metadata_main.c
>  	$(COMPILE) -c -o $@ $<
> -package_metadata_test$(EXEEXT): package_metadata_test.o gcctestdir/ld
> +package_metadata_test1$(EXEEXT): package_metadata_test.o gcctestdir/ld
>  	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='{"foo":"bar"}'
>  	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
> +package_metadata_test1b$(EXEEXT): package_metadata_test.o gcctestdir/ld
> +	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata=%7B%22foo%22%3A%22bar%22%7D
> +	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
> +package_metadata_test1c$(EXEEXT): package_metadata_test.o gcctestdir/ld
> +	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='%[lbrace]%[quot]foo%[quot]:%[quot]bar%[quot]%[rbrace]'
> +	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
> +package_metadata_test2$(EXEEXT): package_metadata_test.o gcctestdir/ld
> +	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata=%7B%22name%22:%22binutils%22%2C%22ver%22%3A%22x%20%%22%7d
> +	$(TEST_READELF) --notes $@ | grep -q '{"name":"binutils","ver":"x %"}'
> +package_metadata_test2b$(EXEEXT): package_metadata_test.o gcctestdir/ld
> +	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='{%[quot]name%[quot]:%[quot]binutils%[quot]%[comma]%[quot]ver%[quot]:%[quot]x%[space]%%[quot]}'
> +	$(TEST_READELF) --notes $@ | grep -q '{"name":"binutils","ver":"x %"}'
> diff --git a/gold/testsuite/Makefile.in b/gold/testsuite/Makefile.in
> index 357dec0d4f9..6cf619fd974 100644
> --- a/gold/testsuite/Makefile.in
> +++ b/gold/testsuite/Makefile.in
> @@ -110,7 +110,11 @@ check_PROGRAMS = $(am__EXEEXT_1) $(am__EXEEXT_2) $(am__EXEEXT_3) \
>  	$(am__EXEEXT_40) $(am__EXEEXT_41) $(am__EXEEXT_42) \
>  	$(am__EXEEXT_43) $(am__EXEEXT_44) $(am__EXEEXT_45) \
>  	$(am__EXEEXT_46) $(am__EXEEXT_47) $(am__EXEEXT_48) \
> -	package_metadata_test$(EXEEXT)
> +	package_metadata_test1$(EXEEXT) \
> +	package_metadata_test1b$(EXEEXT) \
> +	package_metadata_test1c$(EXEEXT) \
> +	package_metadata_test2$(EXEEXT) \
> +	package_metadata_test2b$(EXEEXT)
>  @NATIVE_OR_CROSS_LINKER_TRUE@am__append_1 = object_unittest \
>  @NATIVE_OR_CROSS_LINKER_TRUE@	binary_unittest leb128_unittest \
>  @NATIVE_OR_CROSS_LINKER_TRUE@	overflow_unittest
> @@ -1799,9 +1803,21 @@ overflow_unittest_OBJECTS = $(am_overflow_unittest_OBJECTS)
>  @NATIVE_OR_CROSS_LINKER_TRUE@	$(am__DEPENDENCIES_1)
>  overflow_unittest_LINK = $(CXXLD) $(AM_CXXFLAGS) $(CXXFLAGS) \
>  	$(overflow_unittest_LDFLAGS) $(LDFLAGS) -o $@
> -package_metadata_test_SOURCES = package_metadata_test.c
> -package_metadata_test_OBJECTS = package_metadata_test.$(OBJEXT)
> -package_metadata_test_LDADD = $(LDADD)
> +package_metadata_test1_SOURCES = package_metadata_test1.c
> +package_metadata_test1_OBJECTS = package_metadata_test1.$(OBJEXT)
> +package_metadata_test1_LDADD = $(LDADD)
> +package_metadata_test1b_SOURCES = package_metadata_test1b.c
> +package_metadata_test1b_OBJECTS = package_metadata_test1b.$(OBJEXT)
> +package_metadata_test1b_LDADD = $(LDADD)
> +package_metadata_test1c_SOURCES = package_metadata_test1c.c
> +package_metadata_test1c_OBJECTS = package_metadata_test1c.$(OBJEXT)
> +package_metadata_test1c_LDADD = $(LDADD)
> +package_metadata_test2_SOURCES = package_metadata_test2.c
> +package_metadata_test2_OBJECTS = package_metadata_test2.$(OBJEXT)
> +package_metadata_test2_LDADD = $(LDADD)
> +package_metadata_test2b_SOURCES = package_metadata_test2b.c
> +package_metadata_test2b_OBJECTS = package_metadata_test2b.$(OBJEXT)
> +package_metadata_test2b_LDADD = $(LDADD)
>  permission_test_SOURCES = permission_test.c
>  permission_test_OBJECTS = permission_test.$(OBJEXT)
>  permission_test_LDADD = $(LDADD)
> @@ -2355,7 +2371,9 @@ SOURCES = $(libgoldtest_a_SOURCES) $(aarch64_pr23870_SOURCES) \
>  	$(large_symbol_alignment_SOURCES) $(leb128_unittest_SOURCES) \
>  	local_labels_test.c many_sections_r_test.c \
>  	$(many_sections_test_SOURCES) $(object_unittest_SOURCES) \
> -	$(overflow_unittest_SOURCES) package_metadata_test.c \
> +	$(overflow_unittest_SOURCES) package_metadata_test1.c \
> +	package_metadata_test1b.c package_metadata_test1c.c \
> +	package_metadata_test2.c package_metadata_test2b.c \
>  	permission_test.c $(pie_copyrelocs_test_SOURCES) \
>  	plugin_test_1.c plugin_test_10.c plugin_test_11.c \
>  	plugin_test_12.c plugin_test_2.c plugin_test_3.c \
> @@ -4869,7 +4887,11 @@ distclean-compile:
>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/many_sections_test.Po@am__quote@
>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/object_unittest.Po@am__quote@
>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/overflow_unittest.Po@am__quote@
> -@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test.Po@am__quote@
> +@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test1.Po@am__quote@
> +@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test1b.Po@am__quote@
> +@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test1c.Po@am__quote@
> +@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test2.Po@am__quote@
> +@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test2b.Po@am__quote@
>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/permission_test.Po@am__quote@
>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/pie_copyrelocs_test-pie_copyrelocs_test.Po@am__quote@
>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/plugin_test_1.Po@am__quote@
> @@ -7854,9 +7876,37 @@ aarch64_pr23870.log: aarch64_pr23870$(EXEEXT)
>  	--log-file $$b.log --trs-file $$b.trs \
>  	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
>  	"$$tst" $(AM_TESTS_FD_REDIRECT)
> -package_metadata_test.log: package_metadata_test$(EXEEXT)
> -	@p='package_metadata_test$(EXEEXT)'; \
> -	b='package_metadata_test'; \
> +package_metadata_test1.log: package_metadata_test1$(EXEEXT)
> +	@p='package_metadata_test1$(EXEEXT)'; \
> +	b='package_metadata_test1'; \
> +	$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
> +	--log-file $$b.log --trs-file $$b.trs \
> +	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
> +	"$$tst" $(AM_TESTS_FD_REDIRECT)
> +package_metadata_test1b.log: package_metadata_test1b$(EXEEXT)
> +	@p='package_metadata_test1b$(EXEEXT)'; \
> +	b='package_metadata_test1b'; \
> +	$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
> +	--log-file $$b.log --trs-file $$b.trs \
> +	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
> +	"$$tst" $(AM_TESTS_FD_REDIRECT)
> +package_metadata_test1c.log: package_metadata_test1c$(EXEEXT)
> +	@p='package_metadata_test1c$(EXEEXT)'; \
> +	b='package_metadata_test1c'; \
> +	$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
> +	--log-file $$b.log --trs-file $$b.trs \
> +	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
> +	"$$tst" $(AM_TESTS_FD_REDIRECT)
> +package_metadata_test2.log: package_metadata_test2$(EXEEXT)
> +	@p='package_metadata_test2$(EXEEXT)'; \
> +	b='package_metadata_test2'; \
> +	$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
> +	--log-file $$b.log --trs-file $$b.trs \
> +	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
> +	"$$tst" $(AM_TESTS_FD_REDIRECT)
> +package_metadata_test2b.log: package_metadata_test2b$(EXEEXT)
> +	@p='package_metadata_test2b$(EXEEXT)'; \
> +	b='package_metadata_test2b'; \
>  	$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
>  	--log-file $$b.log --trs-file $$b.trs \
>  	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
> @@ -10521,9 +10571,21 @@ uninstall-am:
>  @DEFAULT_TARGET_X86_64_TRUE@	$(TEST_AS) -o $@ $<
>  package_metadata_test.o: package_metadata_main.c
>  	$(COMPILE) -c -o $@ $<
> -package_metadata_test$(EXEEXT): package_metadata_test.o gcctestdir/ld
> +package_metadata_test1$(EXEEXT): package_metadata_test.o gcctestdir/ld
>  	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='{"foo":"bar"}'
>  	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
> +package_metadata_test1b$(EXEEXT): package_metadata_test.o gcctestdir/ld
> +	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata=%7B%22foo%22%3A%22bar%22%7D
> +	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
> +package_metadata_test1c$(EXEEXT): package_metadata_test.o gcctestdir/ld
> +	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='%[lbrace]%[quot]foo%[quot]:%[quot]bar%[quot]%[rbrace]'
> +	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
> +package_metadata_test2$(EXEEXT): package_metadata_test.o gcctestdir/ld
> +	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata=%7B%22name%22:%22binutils%22%2C%22ver%22%3A%22x%20%%22%7d
> +	$(TEST_READELF) --notes $@ | grep -q '{"name":"binutils","ver":"x %"}'
> +package_metadata_test2b$(EXEEXT): package_metadata_test.o gcctestdir/ld
> +	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='{%[quot]name%[quot]:%[quot]binutils%[quot]%[comma]%[quot]ver%[quot]:%[quot]x%[space]%%[quot]}'
> +	$(TEST_READELF) --notes $@ | grep -q '{"name":"binutils","ver":"x %"}'
>  
>  # Tell versions [3.59,3.63) of GNU make to not export all variables.
>  # Otherwise a system limit (for SysV at least) may be exceeded.
  
Sam James Dec. 17, 2024, 12:27 p.m. UTC | #2
Benjamin Drung <benjamin.drung@canonical.com> writes:

> Friendly ping since I haven't got a response since submission three
> weeks ago.

I recommend CCing the gold maintainers.

>
> On Wed, 2024-11-27 at 10:24 +0000, Benjamin Drung wrote:
>> Specifying the compiler flag `-Wl,--package-metadata=<JSON>` will not
>> work in case the JSON contains a comma, because compiler drivers eat
>> commas. Example:
>> 
>> ```
>> $ echo "void main() { }" > test.c
>> $ gcc -fuse-ld=gold '-Wl,--package-metadata={"type":"deb","os":"ubuntu"}' test.c
>> /usr/bin/ld.gold: error: cannot open "os":"ubuntu"}: No such file or directory
>> /usr/bin/ld.gold: fatal error: error: --package-metadata={"type":"deb" does not contain valid JSON: '}' expected near end of file
>> 
>> collect2: error: ld returned 1 exit status
>> ```
>> 
>> The quotation marks in the JSON value do not work well with shell nor
>> make. Specifying the `--package-metadata` linker flag in a `LDFLAGS`
>> environment variable might loose its quotation marks when it hits the
>> final compiler call.
>> 
>> Following the same format as the implementation in ld:
>> b0cc81e87087bb8a6b12dc1e4fd7f2591927977b
>> 
>> So support percent-encoded and %[string] encoded JSON data in the
>> `--package-metadata` linker flag. Percent-encoding is used because it is
>> a standard, simple to implement, and does take too many additional
>> characters. %[string] encoding is supported for having a more readable
>> encoding.
>> 
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32003
>> Bug-Ubutru: https://bugs.launchpad.net/bugs/2071468
>> Signed-off-by: Benjamin Drung <benjamin.drung@canonical.com>
>> ---
>>  gold/options.cc            | 72 +++++++++++++++++++++++++++++++++
>>  gold/options.h             | 17 +++++++-
>>  gold/testsuite/Makefile.am | 16 +++++++-
>>  gold/testsuite/Makefile.in | 82 +++++++++++++++++++++++++++++++++-----
>>  4 files changed, 174 insertions(+), 13 deletions(-)
>> 
>> diff --git a/gold/options.cc b/gold/options.cc
>> index 46f067fa72f..4f615d44e28 100644
>> --- a/gold/options.cc
>> +++ b/gold/options.cc
>> @@ -255,6 +255,78 @@ parse_string(const char* option_name, const char* arg, const char** retval)
>>    *retval = arg;
>>  }
>>  
>> +// Decode a percent and/or %[string] encoded string. Following %[string]
>> +// encodings are supported:
>> +//
>> +// %[comma] for ,
>> +// %[lbrace] for {
>> +// %[quot] for "
>> +// %[rbrace] for }
>> +// %[space] for ' '
>> +//
>> +// The percent decoding behaves the same as Python's urllib.parse.unquote.
>> +void
>> +parse_optional_encoded_string(const char* option_name, const char* arg, const char** retval)
>> +{
>> +  hex_init();
>> +  size_t arg_len = strlen (arg);
>> +  char *package_metadata = (char *)malloc (arg_len + 1);
>> +  if (package_metadata == NULL)
>> +    gold_fatal(_("%s: malloc failed"), option_name);
>> +
>> +  const char *src = arg;
>> +  char *dst = package_metadata;
>> +  while (*src != '\0')
>> +    {
>> +      char c = *src++;
>> +      if (c == '%')
>> +	{
>> +	  char next1 = *src;
>> +	  if (next1 != '\0' && hex_p(next1))
>> +	    {
>> +	      char next2 = *(src + 1);
>> +	      if (next2 != '\0' && hex_p(next2))
>> +		{
>> +		  c = (char)((hex_value(next1) << 4) + hex_value(next2));
>> +		  src += 2;
>> +		}
>> +	    }
>> +	  else if (next1 == '[')
>> +	    {
>> +	      if (strncmp(src + 1, "comma]", 6) == 0)
>> +		{
>> +		  c = ',';
>> +		  src += 7;
>> +		}
>> +	      else if (strncmp(src + 1, "lbrace]", 7) == 0)
>> +		{
>> +		  c = '{';
>> +		  src += 8;
>> +		}
>> +	      else if (strncmp(src + 1, "quot]", 5) == 0)
>> +		{
>> +		  c = '"';
>> +		  src += 6;
>> +		}
>> +	      else if (strncmp(src + 1, "rbrace]", 7) == 0)
>> +		{
>> +		  c = '}';
>> +		  src += 8;
>> +		}
>> +	      else if (strncmp(src + 1, "space]", 6) == 0)
>> +		{
>> +		  c = ' ';
>> +		  src += 7;
>> +		}
>> +	    }
>> +	}
>> +      *dst++ = c;
>> +    }
>> +  *dst = '\0';
>> +
>> +  *retval = package_metadata;
>> +}
>> +
>>  void
>>  parse_optional_string(const char*, const char* arg, const char** retval)
>>  {
>> diff --git a/gold/options.h b/gold/options.h
>> index 446e8d42614..78766c1fc6b 100644
>> --- a/gold/options.h
>> +++ b/gold/options.h
>> @@ -108,6 +108,10 @@ parse_percent(const char* option_name, const char* arg, double* retval);
>>  extern void
>>  parse_string(const char* option_name, const char* arg, const char** retval);
>>  
>> +extern void
>> +parse_optional_encoded_string(const char* option_name, const char* arg,
>> +			      const char** retval);
>> +
>>  extern void
>>  parse_optional_string(const char* option_name, const char* arg,
>>  		      const char** retval);
>> @@ -589,6 +593,17 @@ struct Struct_special : public Struct_var
>>    };                                                                    \
>>    Struct_##varname__ varname__##_initializer_
>>  
>> +// An option that takes an optional string argument.  If the option is
>> +// used with no argument, the value will be the default, and
>> +// user_set_via_option will be true.
>> +#define DEFINE_optional_encoded_string(varname__, dashes__,		\
>> +				       shortname__, default_value__,	\
>> +				       helpstring__, helparg__)		\
>> +  DEFINE_var(varname__, dashes__, shortname__, default_value__,		\
>> +	     default_value__, helpstring__, helparg__, true,		\
>> +	     const char*, const char*,					\
>> +	     options::parse_optional_encoded_string, false)
>> +
>>  // An option that takes an optional string argument.  If the option is
>>  // used with no argument, the value will be the default, and
>>  // user_set_via_option will be true.
>> @@ -1106,7 +1121,7 @@ class General_options
>>    DEFINE_bool(p, options::ONE_DASH, 'p', false,
>>  	      N_("Ignored for ARM compatibility"), NULL);
>>  
>> -  DEFINE_optional_string(package_metadata, options::TWO_DASHES, '\0', NULL,
>> +  DEFINE_optional_encoded_string(package_metadata, options::TWO_DASHES, '\0', NULL,
>>  			 N_("Generate package metadata note"),
>>  			 N_("[=JSON]"));
>>  
>> diff --git a/gold/testsuite/Makefile.am b/gold/testsuite/Makefile.am
>> index 8f158ba20cc..d7f496c75c1 100644
>> --- a/gold/testsuite/Makefile.am
>> +++ b/gold/testsuite/Makefile.am
>> @@ -4470,9 +4470,21 @@ retain_2.o: retain_2.s
>>  
>>  endif DEFAULT_TARGET_X86_64
>>  
>> -check_PROGRAMS += package_metadata_test
>> +check_PROGRAMS += package_metadata_test1 package_metadata_test1b package_metadata_test1c package_metadata_test2 package_metadata_test2b
>>  package_metadata_test.o: package_metadata_main.c
>>  	$(COMPILE) -c -o $@ $<
>> -package_metadata_test$(EXEEXT): package_metadata_test.o gcctestdir/ld
>> +package_metadata_test1$(EXEEXT): package_metadata_test.o gcctestdir/ld
>>  	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='{"foo":"bar"}'
>>  	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
>> +package_metadata_test1b$(EXEEXT): package_metadata_test.o gcctestdir/ld
>> +	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata=%7B%22foo%22%3A%22bar%22%7D
>> +	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
>> +package_metadata_test1c$(EXEEXT): package_metadata_test.o gcctestdir/ld
>> +	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='%[lbrace]%[quot]foo%[quot]:%[quot]bar%[quot]%[rbrace]'
>> +	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
>> +package_metadata_test2$(EXEEXT): package_metadata_test.o gcctestdir/ld
>> +	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata=%7B%22name%22:%22binutils%22%2C%22ver%22%3A%22x%20%%22%7d
>> +	$(TEST_READELF) --notes $@ | grep -q '{"name":"binutils","ver":"x %"}'
>> +package_metadata_test2b$(EXEEXT): package_metadata_test.o gcctestdir/ld
>> +	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='{%[quot]name%[quot]:%[quot]binutils%[quot]%[comma]%[quot]ver%[quot]:%[quot]x%[space]%%[quot]}'
>> +	$(TEST_READELF) --notes $@ | grep -q '{"name":"binutils","ver":"x %"}'
>> diff --git a/gold/testsuite/Makefile.in b/gold/testsuite/Makefile.in
>> index 357dec0d4f9..6cf619fd974 100644
>> --- a/gold/testsuite/Makefile.in
>> +++ b/gold/testsuite/Makefile.in
>> @@ -110,7 +110,11 @@ check_PROGRAMS = $(am__EXEEXT_1) $(am__EXEEXT_2) $(am__EXEEXT_3) \
>>  	$(am__EXEEXT_40) $(am__EXEEXT_41) $(am__EXEEXT_42) \
>>  	$(am__EXEEXT_43) $(am__EXEEXT_44) $(am__EXEEXT_45) \
>>  	$(am__EXEEXT_46) $(am__EXEEXT_47) $(am__EXEEXT_48) \
>> -	package_metadata_test$(EXEEXT)
>> +	package_metadata_test1$(EXEEXT) \
>> +	package_metadata_test1b$(EXEEXT) \
>> +	package_metadata_test1c$(EXEEXT) \
>> +	package_metadata_test2$(EXEEXT) \
>> +	package_metadata_test2b$(EXEEXT)
>>  @NATIVE_OR_CROSS_LINKER_TRUE@am__append_1 = object_unittest \
>>  @NATIVE_OR_CROSS_LINKER_TRUE@	binary_unittest leb128_unittest \
>>  @NATIVE_OR_CROSS_LINKER_TRUE@	overflow_unittest
>> @@ -1799,9 +1803,21 @@ overflow_unittest_OBJECTS = $(am_overflow_unittest_OBJECTS)
>>  @NATIVE_OR_CROSS_LINKER_TRUE@	$(am__DEPENDENCIES_1)
>>  overflow_unittest_LINK = $(CXXLD) $(AM_CXXFLAGS) $(CXXFLAGS) \
>>  	$(overflow_unittest_LDFLAGS) $(LDFLAGS) -o $@
>> -package_metadata_test_SOURCES = package_metadata_test.c
>> -package_metadata_test_OBJECTS = package_metadata_test.$(OBJEXT)
>> -package_metadata_test_LDADD = $(LDADD)
>> +package_metadata_test1_SOURCES = package_metadata_test1.c
>> +package_metadata_test1_OBJECTS = package_metadata_test1.$(OBJEXT)
>> +package_metadata_test1_LDADD = $(LDADD)
>> +package_metadata_test1b_SOURCES = package_metadata_test1b.c
>> +package_metadata_test1b_OBJECTS = package_metadata_test1b.$(OBJEXT)
>> +package_metadata_test1b_LDADD = $(LDADD)
>> +package_metadata_test1c_SOURCES = package_metadata_test1c.c
>> +package_metadata_test1c_OBJECTS = package_metadata_test1c.$(OBJEXT)
>> +package_metadata_test1c_LDADD = $(LDADD)
>> +package_metadata_test2_SOURCES = package_metadata_test2.c
>> +package_metadata_test2_OBJECTS = package_metadata_test2.$(OBJEXT)
>> +package_metadata_test2_LDADD = $(LDADD)
>> +package_metadata_test2b_SOURCES = package_metadata_test2b.c
>> +package_metadata_test2b_OBJECTS = package_metadata_test2b.$(OBJEXT)
>> +package_metadata_test2b_LDADD = $(LDADD)
>>  permission_test_SOURCES = permission_test.c
>>  permission_test_OBJECTS = permission_test.$(OBJEXT)
>>  permission_test_LDADD = $(LDADD)
>> @@ -2355,7 +2371,9 @@ SOURCES = $(libgoldtest_a_SOURCES) $(aarch64_pr23870_SOURCES) \
>>  	$(large_symbol_alignment_SOURCES) $(leb128_unittest_SOURCES) \
>>  	local_labels_test.c many_sections_r_test.c \
>>  	$(many_sections_test_SOURCES) $(object_unittest_SOURCES) \
>> -	$(overflow_unittest_SOURCES) package_metadata_test.c \
>> +	$(overflow_unittest_SOURCES) package_metadata_test1.c \
>> +	package_metadata_test1b.c package_metadata_test1c.c \
>> +	package_metadata_test2.c package_metadata_test2b.c \
>>  	permission_test.c $(pie_copyrelocs_test_SOURCES) \
>>  	plugin_test_1.c plugin_test_10.c plugin_test_11.c \
>>  	plugin_test_12.c plugin_test_2.c plugin_test_3.c \
>> @@ -4869,7 +4887,11 @@ distclean-compile:
>>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/many_sections_test.Po@am__quote@
>>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/object_unittest.Po@am__quote@
>>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/overflow_unittest.Po@am__quote@
>> -@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test.Po@am__quote@
>> +@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test1.Po@am__quote@
>> +@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test1b.Po@am__quote@
>> +@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test1c.Po@am__quote@
>> +@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test2.Po@am__quote@
>> +@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test2b.Po@am__quote@
>>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/permission_test.Po@am__quote@
>>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/pie_copyrelocs_test-pie_copyrelocs_test.Po@am__quote@
>>  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/plugin_test_1.Po@am__quote@
>> @@ -7854,9 +7876,37 @@ aarch64_pr23870.log: aarch64_pr23870$(EXEEXT)
>>  	--log-file $$b.log --trs-file $$b.trs \
>>  	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
>>  	"$$tst" $(AM_TESTS_FD_REDIRECT)
>> -package_metadata_test.log: package_metadata_test$(EXEEXT)
>> -	@p='package_metadata_test$(EXEEXT)'; \
>> -	b='package_metadata_test'; \
>> +package_metadata_test1.log: package_metadata_test1$(EXEEXT)
>> +	@p='package_metadata_test1$(EXEEXT)'; \
>> +	b='package_metadata_test1'; \
>> +	$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
>> +	--log-file $$b.log --trs-file $$b.trs \
>> +	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
>> +	"$$tst" $(AM_TESTS_FD_REDIRECT)
>> +package_metadata_test1b.log: package_metadata_test1b$(EXEEXT)
>> +	@p='package_metadata_test1b$(EXEEXT)'; \
>> +	b='package_metadata_test1b'; \
>> +	$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
>> +	--log-file $$b.log --trs-file $$b.trs \
>> +	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
>> +	"$$tst" $(AM_TESTS_FD_REDIRECT)
>> +package_metadata_test1c.log: package_metadata_test1c$(EXEEXT)
>> +	@p='package_metadata_test1c$(EXEEXT)'; \
>> +	b='package_metadata_test1c'; \
>> +	$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
>> +	--log-file $$b.log --trs-file $$b.trs \
>> +	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
>> +	"$$tst" $(AM_TESTS_FD_REDIRECT)
>> +package_metadata_test2.log: package_metadata_test2$(EXEEXT)
>> +	@p='package_metadata_test2$(EXEEXT)'; \
>> +	b='package_metadata_test2'; \
>> +	$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
>> +	--log-file $$b.log --trs-file $$b.trs \
>> +	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
>> +	"$$tst" $(AM_TESTS_FD_REDIRECT)
>> +package_metadata_test2b.log: package_metadata_test2b$(EXEEXT)
>> +	@p='package_metadata_test2b$(EXEEXT)'; \
>> +	b='package_metadata_test2b'; \
>>  	$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
>>  	--log-file $$b.log --trs-file $$b.trs \
>>  	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
>> @@ -10521,9 +10571,21 @@ uninstall-am:
>>  @DEFAULT_TARGET_X86_64_TRUE@	$(TEST_AS) -o $@ $<
>>  package_metadata_test.o: package_metadata_main.c
>>  	$(COMPILE) -c -o $@ $<
>> -package_metadata_test$(EXEEXT): package_metadata_test.o gcctestdir/ld
>> +package_metadata_test1$(EXEEXT): package_metadata_test.o gcctestdir/ld
>>  	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='{"foo":"bar"}'
>>  	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
>> +package_metadata_test1b$(EXEEXT): package_metadata_test.o gcctestdir/ld
>> +	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata=%7B%22foo%22%3A%22bar%22%7D
>> +	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
>> +package_metadata_test1c$(EXEEXT): package_metadata_test.o gcctestdir/ld
>> +	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='%[lbrace]%[quot]foo%[quot]:%[quot]bar%[quot]%[rbrace]'
>> +	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
>> +package_metadata_test2$(EXEEXT): package_metadata_test.o gcctestdir/ld
>> +	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata=%7B%22name%22:%22binutils%22%2C%22ver%22%3A%22x%20%%22%7d
>> +	$(TEST_READELF) --notes $@ | grep -q '{"name":"binutils","ver":"x %"}'
>> +package_metadata_test2b$(EXEEXT): package_metadata_test.o gcctestdir/ld
>> +	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='{%[quot]name%[quot]:%[quot]binutils%[quot]%[comma]%[quot]ver%[quot]:%[quot]x%[space]%%[quot]}'
>> +	$(TEST_READELF) --notes $@ | grep -q '{"name":"binutils","ver":"x %"}'
>>  
>>  # Tell versions [3.59,3.63) of GNU make to not export all variables.
>>  # Otherwise a system limit (for SysV at least) may be exceeded.
  
Benjamin Drung Dec. 17, 2024, 3:28 p.m. UTC | #3
On Tue, 2024-12-17 at 12:27 +0000, Sam James wrote:
> Benjamin Drung <benjamin.drung@canonical.com> writes:
> 
> > Friendly ping since I haven't got a response since submission three
> > weeks ago.
> 
> I recommend CCing the gold maintainers.

Thanks. I resent the patch with gdb-patches@ in CC.
> 

> > On Wed, 2024-11-27 at 10:24 +0000, Benjamin Drung wrote:
> > > Specifying the compiler flag `-Wl,--package-metadata=<JSON>` will not
> > > work in case the JSON contains a comma, because compiler drivers eat
> > > commas. Example:
> > > 
> > > ```
> > > $ echo "void main() { }" > test.c
> > > $ gcc -fuse-ld=gold '-Wl,--package-metadata={"type":"deb","os":"ubuntu"}' test.c
> > > /usr/bin/ld.gold: error: cannot open "os":"ubuntu"}: No such file or directory
> > > /usr/bin/ld.gold: fatal error: error: --package-metadata={"type":"deb" does not contain valid JSON: '}' expected near end of file
> > > 
> > > collect2: error: ld returned 1 exit status
> > > ```
> > > 
> > > The quotation marks in the JSON value do not work well with shell nor
> > > make. Specifying the `--package-metadata` linker flag in a `LDFLAGS`
> > > environment variable might loose its quotation marks when it hits the
> > > final compiler call.
> > > 
> > > Following the same format as the implementation in ld:
> > > b0cc81e87087bb8a6b12dc1e4fd7f2591927977b
> > > 
> > > So support percent-encoded and %[string] encoded JSON data in the
> > > `--package-metadata` linker flag. Percent-encoding is used because it is
> > > a standard, simple to implement, and does take too many additional
> > > characters. %[string] encoding is supported for having a more readable
> > > encoding.
> > > 
> > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32003
> > > Bug-Ubutru: https://bugs.launchpad.net/bugs/2071468
> > > Signed-off-by: Benjamin Drung <benjamin.drung@canonical.com>
> > > ---
> > >  gold/options.cc            | 72 +++++++++++++++++++++++++++++++++
> > >  gold/options.h             | 17 +++++++-
> > >  gold/testsuite/Makefile.am | 16 +++++++-
> > >  gold/testsuite/Makefile.in | 82 +++++++++++++++++++++++++++++++++-----
> > >  4 files changed, 174 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/gold/options.cc b/gold/options.cc
> > > index 46f067fa72f..4f615d44e28 100644
> > > --- a/gold/options.cc
> > > +++ b/gold/options.cc
> > > @@ -255,6 +255,78 @@ parse_string(const char* option_name, const char* arg, const char** retval)
> > >    *retval = arg;
> > >  }
> > >  
> > > +// Decode a percent and/or %[string] encoded string. Following %[string]
> > > +// encodings are supported:
> > > +//
> > > +// %[comma] for ,
> > > +// %[lbrace] for {
> > > +// %[quot] for "
> > > +// %[rbrace] for }
> > > +// %[space] for ' '
> > > +//
> > > +// The percent decoding behaves the same as Python's urllib.parse.unquote.
> > > +void
> > > +parse_optional_encoded_string(const char* option_name, const char* arg, const char** retval)
> > > +{
> > > +  hex_init();
> > > +  size_t arg_len = strlen (arg);
> > > +  char *package_metadata = (char *)malloc (arg_len + 1);
> > > +  if (package_metadata == NULL)
> > > +    gold_fatal(_("%s: malloc failed"), option_name);
> > > +
> > > +  const char *src = arg;
> > > +  char *dst = package_metadata;
> > > +  while (*src != '\0')
> > > +    {
> > > +      char c = *src++;
> > > +      if (c == '%')
> > > +	{
> > > +	  char next1 = *src;
> > > +	  if (next1 != '\0' && hex_p(next1))
> > > +	    {
> > > +	      char next2 = *(src + 1);
> > > +	      if (next2 != '\0' && hex_p(next2))
> > > +		{
> > > +		  c = (char)((hex_value(next1) << 4) + hex_value(next2));
> > > +		  src += 2;
> > > +		}
> > > +	    }
> > > +	  else if (next1 == '[')
> > > +	    {
> > > +	      if (strncmp(src + 1, "comma]", 6) == 0)
> > > +		{
> > > +		  c = ',';
> > > +		  src += 7;
> > > +		}
> > > +	      else if (strncmp(src + 1, "lbrace]", 7) == 0)
> > > +		{
> > > +		  c = '{';
> > > +		  src += 8;
> > > +		}
> > > +	      else if (strncmp(src + 1, "quot]", 5) == 0)
> > > +		{
> > > +		  c = '"';
> > > +		  src += 6;
> > > +		}
> > > +	      else if (strncmp(src + 1, "rbrace]", 7) == 0)
> > > +		{
> > > +		  c = '}';
> > > +		  src += 8;
> > > +		}
> > > +	      else if (strncmp(src + 1, "space]", 6) == 0)
> > > +		{
> > > +		  c = ' ';
> > > +		  src += 7;
> > > +		}
> > > +	    }
> > > +	}
> > > +      *dst++ = c;
> > > +    }
> > > +  *dst = '\0';
> > > +
> > > +  *retval = package_metadata;
> > > +}
> > > +
> > >  void
> > >  parse_optional_string(const char*, const char* arg, const char** retval)
> > >  {
> > > diff --git a/gold/options.h b/gold/options.h
> > > index 446e8d42614..78766c1fc6b 100644
> > > --- a/gold/options.h
> > > +++ b/gold/options.h
> > > @@ -108,6 +108,10 @@ parse_percent(const char* option_name, const char* arg, double* retval);
> > >  extern void
> > >  parse_string(const char* option_name, const char* arg, const char** retval);
> > >  
> > > +extern void
> > > +parse_optional_encoded_string(const char* option_name, const char* arg,
> > > +			      const char** retval);
> > > +
> > >  extern void
> > >  parse_optional_string(const char* option_name, const char* arg,
> > >  		      const char** retval);
> > > @@ -589,6 +593,17 @@ struct Struct_special : public Struct_var
> > >    };                                                                    \
> > >    Struct_##varname__ varname__##_initializer_
> > >  
> > > +// An option that takes an optional string argument.  If the option is
> > > +// used with no argument, the value will be the default, and
> > > +// user_set_via_option will be true.
> > > +#define DEFINE_optional_encoded_string(varname__, dashes__,		\
> > > +				       shortname__, default_value__,	\
> > > +				       helpstring__, helparg__)		\
> > > +  DEFINE_var(varname__, dashes__, shortname__, default_value__,		\
> > > +	     default_value__, helpstring__, helparg__, true,		\
> > > +	     const char*, const char*,					\
> > > +	     options::parse_optional_encoded_string, false)
> > > +
> > >  // An option that takes an optional string argument.  If the option is
> > >  // used with no argument, the value will be the default, and
> > >  // user_set_via_option will be true.
> > > @@ -1106,7 +1121,7 @@ class General_options
> > >    DEFINE_bool(p, options::ONE_DASH, 'p', false,
> > >  	      N_("Ignored for ARM compatibility"), NULL);
> > >  
> > > -  DEFINE_optional_string(package_metadata, options::TWO_DASHES, '\0', NULL,
> > > +  DEFINE_optional_encoded_string(package_metadata, options::TWO_DASHES, '\0', NULL,
> > >  			 N_("Generate package metadata note"),
> > >  			 N_("[=JSON]"));
> > >  
> > > diff --git a/gold/testsuite/Makefile.am b/gold/testsuite/Makefile.am
> > > index 8f158ba20cc..d7f496c75c1 100644
> > > --- a/gold/testsuite/Makefile.am
> > > +++ b/gold/testsuite/Makefile.am
> > > @@ -4470,9 +4470,21 @@ retain_2.o: retain_2.s
> > >  
> > >  endif DEFAULT_TARGET_X86_64
> > >  
> > > -check_PROGRAMS += package_metadata_test
> > > +check_PROGRAMS += package_metadata_test1 package_metadata_test1b package_metadata_test1c package_metadata_test2 package_metadata_test2b
> > >  package_metadata_test.o: package_metadata_main.c
> > >  	$(COMPILE) -c -o $@ $<
> > > -package_metadata_test$(EXEEXT): package_metadata_test.o gcctestdir/ld
> > > +package_metadata_test1$(EXEEXT): package_metadata_test.o gcctestdir/ld
> > >  	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='{"foo":"bar"}'
> > >  	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
> > > +package_metadata_test1b$(EXEEXT): package_metadata_test.o gcctestdir/ld
> > > +	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata=%7B%22foo%22%3A%22bar%22%7D
> > > +	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
> > > +package_metadata_test1c$(EXEEXT): package_metadata_test.o gcctestdir/ld
> > > +	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='%[lbrace]%[quot]foo%[quot]:%[quot]bar%[quot]%[rbrace]'
> > > +	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
> > > +package_metadata_test2$(EXEEXT): package_metadata_test.o gcctestdir/ld
> > > +	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata=%7B%22name%22:%22binutils%22%2C%22ver%22%3A%22x%20%%22%7d
> > > +	$(TEST_READELF) --notes $@ | grep -q '{"name":"binutils","ver":"x %"}'
> > > +package_metadata_test2b$(EXEEXT): package_metadata_test.o gcctestdir/ld
> > > +	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='{%[quot]name%[quot]:%[quot]binutils%[quot]%[comma]%[quot]ver%[quot]:%[quot]x%[space]%%[quot]}'
> > > +	$(TEST_READELF) --notes $@ | grep -q '{"name":"binutils","ver":"x %"}'
> > > diff --git a/gold/testsuite/Makefile.in b/gold/testsuite/Makefile.in
> > > index 357dec0d4f9..6cf619fd974 100644
> > > --- a/gold/testsuite/Makefile.in
> > > +++ b/gold/testsuite/Makefile.in
> > > @@ -110,7 +110,11 @@ check_PROGRAMS = $(am__EXEEXT_1) $(am__EXEEXT_2) $(am__EXEEXT_3) \
> > >  	$(am__EXEEXT_40) $(am__EXEEXT_41) $(am__EXEEXT_42) \
> > >  	$(am__EXEEXT_43) $(am__EXEEXT_44) $(am__EXEEXT_45) \
> > >  	$(am__EXEEXT_46) $(am__EXEEXT_47) $(am__EXEEXT_48) \
> > > -	package_metadata_test$(EXEEXT)
> > > +	package_metadata_test1$(EXEEXT) \
> > > +	package_metadata_test1b$(EXEEXT) \
> > > +	package_metadata_test1c$(EXEEXT) \
> > > +	package_metadata_test2$(EXEEXT) \
> > > +	package_metadata_test2b$(EXEEXT)
> > >  @NATIVE_OR_CROSS_LINKER_TRUE@am__append_1 = object_unittest \
> > >  @NATIVE_OR_CROSS_LINKER_TRUE@	binary_unittest leb128_unittest \
> > >  @NATIVE_OR_CROSS_LINKER_TRUE@	overflow_unittest
> > > @@ -1799,9 +1803,21 @@ overflow_unittest_OBJECTS = $(am_overflow_unittest_OBJECTS)
> > >  @NATIVE_OR_CROSS_LINKER_TRUE@	$(am__DEPENDENCIES_1)
> > >  overflow_unittest_LINK = $(CXXLD) $(AM_CXXFLAGS) $(CXXFLAGS) \
> > >  	$(overflow_unittest_LDFLAGS) $(LDFLAGS) -o $@
> > > -package_metadata_test_SOURCES = package_metadata_test.c
> > > -package_metadata_test_OBJECTS = package_metadata_test.$(OBJEXT)
> > > -package_metadata_test_LDADD = $(LDADD)
> > > +package_metadata_test1_SOURCES = package_metadata_test1.c
> > > +package_metadata_test1_OBJECTS = package_metadata_test1.$(OBJEXT)
> > > +package_metadata_test1_LDADD = $(LDADD)
> > > +package_metadata_test1b_SOURCES = package_metadata_test1b.c
> > > +package_metadata_test1b_OBJECTS = package_metadata_test1b.$(OBJEXT)
> > > +package_metadata_test1b_LDADD = $(LDADD)
> > > +package_metadata_test1c_SOURCES = package_metadata_test1c.c
> > > +package_metadata_test1c_OBJECTS = package_metadata_test1c.$(OBJEXT)
> > > +package_metadata_test1c_LDADD = $(LDADD)
> > > +package_metadata_test2_SOURCES = package_metadata_test2.c
> > > +package_metadata_test2_OBJECTS = package_metadata_test2.$(OBJEXT)
> > > +package_metadata_test2_LDADD = $(LDADD)
> > > +package_metadata_test2b_SOURCES = package_metadata_test2b.c
> > > +package_metadata_test2b_OBJECTS = package_metadata_test2b.$(OBJEXT)
> > > +package_metadata_test2b_LDADD = $(LDADD)
> > >  permission_test_SOURCES = permission_test.c
> > >  permission_test_OBJECTS = permission_test.$(OBJEXT)
> > >  permission_test_LDADD = $(LDADD)
> > > @@ -2355,7 +2371,9 @@ SOURCES = $(libgoldtest_a_SOURCES) $(aarch64_pr23870_SOURCES) \
> > >  	$(large_symbol_alignment_SOURCES) $(leb128_unittest_SOURCES) \
> > >  	local_labels_test.c many_sections_r_test.c \
> > >  	$(many_sections_test_SOURCES) $(object_unittest_SOURCES) \
> > > -	$(overflow_unittest_SOURCES) package_metadata_test.c \
> > > +	$(overflow_unittest_SOURCES) package_metadata_test1.c \
> > > +	package_metadata_test1b.c package_metadata_test1c.c \
> > > +	package_metadata_test2.c package_metadata_test2b.c \
> > >  	permission_test.c $(pie_copyrelocs_test_SOURCES) \
> > >  	plugin_test_1.c plugin_test_10.c plugin_test_11.c \
> > >  	plugin_test_12.c plugin_test_2.c plugin_test_3.c \
> > > @@ -4869,7 +4887,11 @@ distclean-compile:
> > >  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/many_sections_test.Po@am__quote@
> > >  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/object_unittest.Po@am__quote@
> > >  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/overflow_unittest.Po@am__quote@
> > > -@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test.Po@am__quote@
> > > +@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test1.Po@am__quote@
> > > +@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test1b.Po@am__quote@
> > > +@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test1c.Po@am__quote@
> > > +@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test2.Po@am__quote@
> > > +@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test2b.Po@am__quote@
> > >  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/permission_test.Po@am__quote@
> > >  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/pie_copyrelocs_test-pie_copyrelocs_test.Po@am__quote@
> > >  @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/plugin_test_1.Po@am__quote@
> > > @@ -7854,9 +7876,37 @@ aarch64_pr23870.log: aarch64_pr23870$(EXEEXT)
> > >  	--log-file $$b.log --trs-file $$b.trs \
> > >  	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
> > >  	"$$tst" $(AM_TESTS_FD_REDIRECT)
> > > -package_metadata_test.log: package_metadata_test$(EXEEXT)
> > > -	@p='package_metadata_test$(EXEEXT)'; \
> > > -	b='package_metadata_test'; \
> > > +package_metadata_test1.log: package_metadata_test1$(EXEEXT)
> > > +	@p='package_metadata_test1$(EXEEXT)'; \
> > > +	b='package_metadata_test1'; \
> > > +	$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
> > > +	--log-file $$b.log --trs-file $$b.trs \
> > > +	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
> > > +	"$$tst" $(AM_TESTS_FD_REDIRECT)
> > > +package_metadata_test1b.log: package_metadata_test1b$(EXEEXT)
> > > +	@p='package_metadata_test1b$(EXEEXT)'; \
> > > +	b='package_metadata_test1b'; \
> > > +	$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
> > > +	--log-file $$b.log --trs-file $$b.trs \
> > > +	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
> > > +	"$$tst" $(AM_TESTS_FD_REDIRECT)
> > > +package_metadata_test1c.log: package_metadata_test1c$(EXEEXT)
> > > +	@p='package_metadata_test1c$(EXEEXT)'; \
> > > +	b='package_metadata_test1c'; \
> > > +	$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
> > > +	--log-file $$b.log --trs-file $$b.trs \
> > > +	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
> > > +	"$$tst" $(AM_TESTS_FD_REDIRECT)
> > > +package_metadata_test2.log: package_metadata_test2$(EXEEXT)
> > > +	@p='package_metadata_test2$(EXEEXT)'; \
> > > +	b='package_metadata_test2'; \
> > > +	$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
> > > +	--log-file $$b.log --trs-file $$b.trs \
> > > +	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
> > > +	"$$tst" $(AM_TESTS_FD_REDIRECT)
> > > +package_metadata_test2b.log: package_metadata_test2b$(EXEEXT)
> > > +	@p='package_metadata_test2b$(EXEEXT)'; \
> > > +	b='package_metadata_test2b'; \
> > >  	$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
> > >  	--log-file $$b.log --trs-file $$b.trs \
> > >  	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
> > > @@ -10521,9 +10571,21 @@ uninstall-am:
> > >  @DEFAULT_TARGET_X86_64_TRUE@	$(TEST_AS) -o $@ $<
> > >  package_metadata_test.o: package_metadata_main.c
> > >  	$(COMPILE) -c -o $@ $<
> > > -package_metadata_test$(EXEEXT): package_metadata_test.o gcctestdir/ld
> > > +package_metadata_test1$(EXEEXT): package_metadata_test.o gcctestdir/ld
> > >  	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='{"foo":"bar"}'
> > >  	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
> > > +package_metadata_test1b$(EXEEXT): package_metadata_test.o gcctestdir/ld
> > > +	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata=%7B%22foo%22%3A%22bar%22%7D
> > > +	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
> > > +package_metadata_test1c$(EXEEXT): package_metadata_test.o gcctestdir/ld
> > > +	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='%[lbrace]%[quot]foo%[quot]:%[quot]bar%[quot]%[rbrace]'
> > > +	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
> > > +package_metadata_test2$(EXEEXT): package_metadata_test.o gcctestdir/ld
> > > +	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata=%7B%22name%22:%22binutils%22%2C%22ver%22%3A%22x%20%%22%7d
> > > +	$(TEST_READELF) --notes $@ | grep -q '{"name":"binutils","ver":"x %"}'
> > > +package_metadata_test2b$(EXEEXT): package_metadata_test.o gcctestdir/ld
> > > +	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='{%[quot]name%[quot]:%[quot]binutils%[quot]%[comma]%[quot]ver%[quot]:%[quot]x%[space]%%[quot]}'
> > > +	$(TEST_READELF) --notes $@ | grep -q '{"name":"binutils","ver":"x %"}'
> > >  
> > >  # Tell versions [3.59,3.63) of GNU make to not export all variables.
> > >  # Otherwise a system limit (for SysV at least) may be exceeded.
  
Matthias Klose Dec. 19, 2024, 9:14 a.m. UTC | #4
On 17.12.24 13:27, Sam James wrote:
> Benjamin Drung <benjamin.drung@canonical.com> writes:
> 
>> Friendly ping since I haven't got a response since submission three
>> weeks ago.
> 
> I recommend CCing the gold maintainers.

that raises the question, who are these maintainers these days. CCing 
Cary here, but looking at the gold commits, gold seems to be mostly in 
maintenance mode.

Matthias
  
Jan Beulich Dec. 19, 2024, 9:18 a.m. UTC | #5
On 19.12.2024 10:14, Matthias Klose wrote:
> On 17.12.24 13:27, Sam James wrote:
>> Benjamin Drung <benjamin.drung@canonical.com> writes:
>>
>>> Friendly ping since I haven't got a response since submission three
>>> weeks ago.
>>
>> I recommend CCing the gold maintainers.
> 
> that raises the question, who are these maintainers these days. CCing 
> Cary here, but looking at the gold commits, gold seems to be mostly in 
> maintenance mode.

Irrespective of that binutils/MAINTAINERS is still the canonical reference
to determine who's the maintainer for what. If entries there look
inappropriate, patches will need proposing.

Jan
  
Benjamin Drung Dec. 19, 2024, 10:53 a.m. UTC | #6
On Thu, 2024-12-19 at 10:18 +0100, Jan Beulich wrote:
> On 19.12.2024 10:14, Matthias Klose wrote:
> > On 17.12.24 13:27, Sam James wrote:
> > > Benjamin Drung <benjamin.drung@canonical.com> writes:
> > > 
> > > > Friendly ping since I haven't got a response since submission three
> > > > weeks ago.
> > > 
> > > I recommend CCing the gold maintainers.
> > 
> > that raises the question, who are these maintainers these days. CCing 
> > Cary here, but looking at the gold commits, gold seems to be mostly in 
> > maintenance mode.
> 
> Irrespective of that binutils/MAINTAINERS is still the canonical reference
> to determine who's the maintainer for what. If entries there look
> inappropriate, patches will need proposing.

And CCing Ian Lance Taylor after seeing binutils/MAINTAINERS.

It would be useful to have gold/MAINTAINERS pointing to
binutils/MAINTAINERS for reference. gold/README says: "See
../binutils/README for more general notes, including where to send bug
reports." but gold/README does not talk about where to send patches to.
  
Ian Lance Taylor Dec. 19, 2024, 2:34 p.m. UTC | #7
On Thu, Dec 19, 2024 at 2:53 AM Benjamin Drung
<benjamin.drung@canonical.com> wrote:
>
> On Thu, 2024-12-19 at 10:18 +0100, Jan Beulich wrote:
> > On 19.12.2024 10:14, Matthias Klose wrote:
> > > On 17.12.24 13:27, Sam James wrote:
> > > > Benjamin Drung <benjamin.drung@canonical.com> writes:
> > > >
> > > > > Friendly ping since I haven't got a response since submission three
> > > > > weeks ago.
> > > >
> > > > I recommend CCing the gold maintainers.
> > >
> > > that raises the question, who are these maintainers these days. CCing
> > > Cary here, but looking at the gold commits, gold seems to be mostly in
> > > maintenance mode.
> >
> > Irrespective of that binutils/MAINTAINERS is still the canonical reference
> > to determine who's the maintainer for what. If entries there look
> > inappropriate, patches will need proposing.
>
> And CCing Ian Lance Taylor after seeing binutils/MAINTAINERS.
>
> It would be useful to have gold/MAINTAINERS pointing to
> binutils/MAINTAINERS for reference. gold/README says: "See
> ../binutils/README for more general notes, including where to send bug
> reports." but gold/README does not talk about where to send patches to.

This list, binutils@sourceware.org, is the right place.

That said I'm not tracking the binutils these days.  This patch seems
fine if it does the same thing as GNU ld.

Ian
  
Benjamin Drung Dec. 19, 2024, 3:25 p.m. UTC | #8
On Thu, 2024-12-19 at 06:34 -0800, Ian Lance Taylor wrote:
> On Thu, Dec 19, 2024 at 2:53 AM Benjamin Drung
> <benjamin.drung@canonical.com> wrote:
> > 
> > On Thu, 2024-12-19 at 10:18 +0100, Jan Beulich wrote:
> > > On 19.12.2024 10:14, Matthias Klose wrote:
> > > > On 17.12.24 13:27, Sam James wrote:
> > > > > Benjamin Drung <benjamin.drung@canonical.com> writes:
> > > > > 
> > > > > > Friendly ping since I haven't got a response since submission three
> > > > > > weeks ago.
> > > > > 
> > > > > I recommend CCing the gold maintainers.
> > > > 
> > > > that raises the question, who are these maintainers these days. CCing
> > > > Cary here, but looking at the gold commits, gold seems to be mostly in
> > > > maintenance mode.
> > > 
> > > Irrespective of that binutils/MAINTAINERS is still the canonical reference
> > > to determine who's the maintainer for what. If entries there look
> > > inappropriate, patches will need proposing.
> > 
> > And CCing Ian Lance Taylor after seeing binutils/MAINTAINERS.
> > 
> > It would be useful to have gold/MAINTAINERS pointing to
> > binutils/MAINTAINERS for reference. gold/README says: "See
> > ../binutils/README for more general notes, including where to send bug
> > reports." but gold/README does not talk about where to send patches to.
> 
> This list, binutils@sourceware.org, is the right place.
> 
> That said I'm not tracking the binutils these days.  This patch seems
> fine if it does the same thing as GNU ld.

Yes, this patch does the same as GNU ld and adds the same test cases as
GNU ld.
  
Cary Coutant Jan. 2, 2025, 11:47 p.m. UTC | #9
> > Benjamin Drung <benjamin.drung@canonical.com> writes:
> >
> >> Friendly ping since I haven't got a response since submission three
> >> weeks ago.
> >
> > I recommend CCing the gold maintainers.
>
> that raises the question, who are these maintainers these days. CCing
> Cary here, but looking at the gold commits, gold seems to be mostly in
> maintenance mode.

Sorry, for various reasons, I haven't been checking binutils often,
and I've been neglectful of my duties here. I'm immensely grateful to
those who have stepped up to approve and apply patches in my absence.
I hope to get back to working on gold (and dwp) again, but it would be
useful to designate someone officially as an additional gold
maintainer.

-cary
  
Benjamin Drung Jan. 10, 2025, 12:54 p.m. UTC | #10
On Thu, 2025-01-02 at 15:47 -0800, Cary Coutant wrote:
> > > Benjamin Drung <benjamin.drung@canonical.com> writes:
> > > 
> > > > Friendly ping since I haven't got a response since submission three
> > > > weeks ago.
> > > 
> > > I recommend CCing the gold maintainers.
> > 
> > that raises the question, who are these maintainers these days. CCing
> > Cary here, but looking at the gold commits, gold seems to be mostly in
> > maintenance mode.
> 
> Sorry, for various reasons, I haven't been checking binutils often,
> and I've been neglectful of my duties here. I'm immensely grateful to
> those who have stepped up to approve and apply patches in my absence.
> I hope to get back to working on gold (and dwp) again, but it would be
> useful to designate someone officially as an additional gold
> maintainer.

As of today, the gold patch for percent-encoded JSON in --package-
metadata hasn't been merged. So consider this a year 2025 ping.
  

Patch

diff --git a/gold/options.cc b/gold/options.cc
index 46f067fa72f..4f615d44e28 100644
--- a/gold/options.cc
+++ b/gold/options.cc
@@ -255,6 +255,78 @@  parse_string(const char* option_name, const char* arg, const char** retval)
   *retval = arg;
 }
 
+// Decode a percent and/or %[string] encoded string. Following %[string]
+// encodings are supported:
+//
+// %[comma] for ,
+// %[lbrace] for {
+// %[quot] for "
+// %[rbrace] for }
+// %[space] for ' '
+//
+// The percent decoding behaves the same as Python's urllib.parse.unquote.
+void
+parse_optional_encoded_string(const char* option_name, const char* arg, const char** retval)
+{
+  hex_init();
+  size_t arg_len = strlen (arg);
+  char *package_metadata = (char *)malloc (arg_len + 1);
+  if (package_metadata == NULL)
+    gold_fatal(_("%s: malloc failed"), option_name);
+
+  const char *src = arg;
+  char *dst = package_metadata;
+  while (*src != '\0')
+    {
+      char c = *src++;
+      if (c == '%')
+	{
+	  char next1 = *src;
+	  if (next1 != '\0' && hex_p(next1))
+	    {
+	      char next2 = *(src + 1);
+	      if (next2 != '\0' && hex_p(next2))
+		{
+		  c = (char)((hex_value(next1) << 4) + hex_value(next2));
+		  src += 2;
+		}
+	    }
+	  else if (next1 == '[')
+	    {
+	      if (strncmp(src + 1, "comma]", 6) == 0)
+		{
+		  c = ',';
+		  src += 7;
+		}
+	      else if (strncmp(src + 1, "lbrace]", 7) == 0)
+		{
+		  c = '{';
+		  src += 8;
+		}
+	      else if (strncmp(src + 1, "quot]", 5) == 0)
+		{
+		  c = '"';
+		  src += 6;
+		}
+	      else if (strncmp(src + 1, "rbrace]", 7) == 0)
+		{
+		  c = '}';
+		  src += 8;
+		}
+	      else if (strncmp(src + 1, "space]", 6) == 0)
+		{
+		  c = ' ';
+		  src += 7;
+		}
+	    }
+	}
+      *dst++ = c;
+    }
+  *dst = '\0';
+
+  *retval = package_metadata;
+}
+
 void
 parse_optional_string(const char*, const char* arg, const char** retval)
 {
diff --git a/gold/options.h b/gold/options.h
index 446e8d42614..78766c1fc6b 100644
--- a/gold/options.h
+++ b/gold/options.h
@@ -108,6 +108,10 @@  parse_percent(const char* option_name, const char* arg, double* retval);
 extern void
 parse_string(const char* option_name, const char* arg, const char** retval);
 
+extern void
+parse_optional_encoded_string(const char* option_name, const char* arg,
+			      const char** retval);
+
 extern void
 parse_optional_string(const char* option_name, const char* arg,
 		      const char** retval);
@@ -589,6 +593,17 @@  struct Struct_special : public Struct_var
   };                                                                    \
   Struct_##varname__ varname__##_initializer_
 
+// An option that takes an optional string argument.  If the option is
+// used with no argument, the value will be the default, and
+// user_set_via_option will be true.
+#define DEFINE_optional_encoded_string(varname__, dashes__,		\
+				       shortname__, default_value__,	\
+				       helpstring__, helparg__)		\
+  DEFINE_var(varname__, dashes__, shortname__, default_value__,		\
+	     default_value__, helpstring__, helparg__, true,		\
+	     const char*, const char*,					\
+	     options::parse_optional_encoded_string, false)
+
 // An option that takes an optional string argument.  If the option is
 // used with no argument, the value will be the default, and
 // user_set_via_option will be true.
@@ -1106,7 +1121,7 @@  class General_options
   DEFINE_bool(p, options::ONE_DASH, 'p', false,
 	      N_("Ignored for ARM compatibility"), NULL);
 
-  DEFINE_optional_string(package_metadata, options::TWO_DASHES, '\0', NULL,
+  DEFINE_optional_encoded_string(package_metadata, options::TWO_DASHES, '\0', NULL,
 			 N_("Generate package metadata note"),
 			 N_("[=JSON]"));
 
diff --git a/gold/testsuite/Makefile.am b/gold/testsuite/Makefile.am
index 8f158ba20cc..d7f496c75c1 100644
--- a/gold/testsuite/Makefile.am
+++ b/gold/testsuite/Makefile.am
@@ -4470,9 +4470,21 @@  retain_2.o: retain_2.s
 
 endif DEFAULT_TARGET_X86_64
 
-check_PROGRAMS += package_metadata_test
+check_PROGRAMS += package_metadata_test1 package_metadata_test1b package_metadata_test1c package_metadata_test2 package_metadata_test2b
 package_metadata_test.o: package_metadata_main.c
 	$(COMPILE) -c -o $@ $<
-package_metadata_test$(EXEEXT): package_metadata_test.o gcctestdir/ld
+package_metadata_test1$(EXEEXT): package_metadata_test.o gcctestdir/ld
 	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='{"foo":"bar"}'
 	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
+package_metadata_test1b$(EXEEXT): package_metadata_test.o gcctestdir/ld
+	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata=%7B%22foo%22%3A%22bar%22%7D
+	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
+package_metadata_test1c$(EXEEXT): package_metadata_test.o gcctestdir/ld
+	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='%[lbrace]%[quot]foo%[quot]:%[quot]bar%[quot]%[rbrace]'
+	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
+package_metadata_test2$(EXEEXT): package_metadata_test.o gcctestdir/ld
+	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata=%7B%22name%22:%22binutils%22%2C%22ver%22%3A%22x%20%%22%7d
+	$(TEST_READELF) --notes $@ | grep -q '{"name":"binutils","ver":"x %"}'
+package_metadata_test2b$(EXEEXT): package_metadata_test.o gcctestdir/ld
+	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='{%[quot]name%[quot]:%[quot]binutils%[quot]%[comma]%[quot]ver%[quot]:%[quot]x%[space]%%[quot]}'
+	$(TEST_READELF) --notes $@ | grep -q '{"name":"binutils","ver":"x %"}'
diff --git a/gold/testsuite/Makefile.in b/gold/testsuite/Makefile.in
index 357dec0d4f9..6cf619fd974 100644
--- a/gold/testsuite/Makefile.in
+++ b/gold/testsuite/Makefile.in
@@ -110,7 +110,11 @@  check_PROGRAMS = $(am__EXEEXT_1) $(am__EXEEXT_2) $(am__EXEEXT_3) \
 	$(am__EXEEXT_40) $(am__EXEEXT_41) $(am__EXEEXT_42) \
 	$(am__EXEEXT_43) $(am__EXEEXT_44) $(am__EXEEXT_45) \
 	$(am__EXEEXT_46) $(am__EXEEXT_47) $(am__EXEEXT_48) \
-	package_metadata_test$(EXEEXT)
+	package_metadata_test1$(EXEEXT) \
+	package_metadata_test1b$(EXEEXT) \
+	package_metadata_test1c$(EXEEXT) \
+	package_metadata_test2$(EXEEXT) \
+	package_metadata_test2b$(EXEEXT)
 @NATIVE_OR_CROSS_LINKER_TRUE@am__append_1 = object_unittest \
 @NATIVE_OR_CROSS_LINKER_TRUE@	binary_unittest leb128_unittest \
 @NATIVE_OR_CROSS_LINKER_TRUE@	overflow_unittest
@@ -1799,9 +1803,21 @@  overflow_unittest_OBJECTS = $(am_overflow_unittest_OBJECTS)
 @NATIVE_OR_CROSS_LINKER_TRUE@	$(am__DEPENDENCIES_1)
 overflow_unittest_LINK = $(CXXLD) $(AM_CXXFLAGS) $(CXXFLAGS) \
 	$(overflow_unittest_LDFLAGS) $(LDFLAGS) -o $@
-package_metadata_test_SOURCES = package_metadata_test.c
-package_metadata_test_OBJECTS = package_metadata_test.$(OBJEXT)
-package_metadata_test_LDADD = $(LDADD)
+package_metadata_test1_SOURCES = package_metadata_test1.c
+package_metadata_test1_OBJECTS = package_metadata_test1.$(OBJEXT)
+package_metadata_test1_LDADD = $(LDADD)
+package_metadata_test1b_SOURCES = package_metadata_test1b.c
+package_metadata_test1b_OBJECTS = package_metadata_test1b.$(OBJEXT)
+package_metadata_test1b_LDADD = $(LDADD)
+package_metadata_test1c_SOURCES = package_metadata_test1c.c
+package_metadata_test1c_OBJECTS = package_metadata_test1c.$(OBJEXT)
+package_metadata_test1c_LDADD = $(LDADD)
+package_metadata_test2_SOURCES = package_metadata_test2.c
+package_metadata_test2_OBJECTS = package_metadata_test2.$(OBJEXT)
+package_metadata_test2_LDADD = $(LDADD)
+package_metadata_test2b_SOURCES = package_metadata_test2b.c
+package_metadata_test2b_OBJECTS = package_metadata_test2b.$(OBJEXT)
+package_metadata_test2b_LDADD = $(LDADD)
 permission_test_SOURCES = permission_test.c
 permission_test_OBJECTS = permission_test.$(OBJEXT)
 permission_test_LDADD = $(LDADD)
@@ -2355,7 +2371,9 @@  SOURCES = $(libgoldtest_a_SOURCES) $(aarch64_pr23870_SOURCES) \
 	$(large_symbol_alignment_SOURCES) $(leb128_unittest_SOURCES) \
 	local_labels_test.c many_sections_r_test.c \
 	$(many_sections_test_SOURCES) $(object_unittest_SOURCES) \
-	$(overflow_unittest_SOURCES) package_metadata_test.c \
+	$(overflow_unittest_SOURCES) package_metadata_test1.c \
+	package_metadata_test1b.c package_metadata_test1c.c \
+	package_metadata_test2.c package_metadata_test2b.c \
 	permission_test.c $(pie_copyrelocs_test_SOURCES) \
 	plugin_test_1.c plugin_test_10.c plugin_test_11.c \
 	plugin_test_12.c plugin_test_2.c plugin_test_3.c \
@@ -4869,7 +4887,11 @@  distclean-compile:
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/many_sections_test.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/object_unittest.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/overflow_unittest.Po@am__quote@
-@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test.Po@am__quote@
+@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test1.Po@am__quote@
+@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test1b.Po@am__quote@
+@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test1c.Po@am__quote@
+@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test2.Po@am__quote@
+@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/package_metadata_test2b.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/permission_test.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/pie_copyrelocs_test-pie_copyrelocs_test.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/plugin_test_1.Po@am__quote@
@@ -7854,9 +7876,37 @@  aarch64_pr23870.log: aarch64_pr23870$(EXEEXT)
 	--log-file $$b.log --trs-file $$b.trs \
 	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
 	"$$tst" $(AM_TESTS_FD_REDIRECT)
-package_metadata_test.log: package_metadata_test$(EXEEXT)
-	@p='package_metadata_test$(EXEEXT)'; \
-	b='package_metadata_test'; \
+package_metadata_test1.log: package_metadata_test1$(EXEEXT)
+	@p='package_metadata_test1$(EXEEXT)'; \
+	b='package_metadata_test1'; \
+	$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
+	--log-file $$b.log --trs-file $$b.trs \
+	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
+	"$$tst" $(AM_TESTS_FD_REDIRECT)
+package_metadata_test1b.log: package_metadata_test1b$(EXEEXT)
+	@p='package_metadata_test1b$(EXEEXT)'; \
+	b='package_metadata_test1b'; \
+	$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
+	--log-file $$b.log --trs-file $$b.trs \
+	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
+	"$$tst" $(AM_TESTS_FD_REDIRECT)
+package_metadata_test1c.log: package_metadata_test1c$(EXEEXT)
+	@p='package_metadata_test1c$(EXEEXT)'; \
+	b='package_metadata_test1c'; \
+	$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
+	--log-file $$b.log --trs-file $$b.trs \
+	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
+	"$$tst" $(AM_TESTS_FD_REDIRECT)
+package_metadata_test2.log: package_metadata_test2$(EXEEXT)
+	@p='package_metadata_test2$(EXEEXT)'; \
+	b='package_metadata_test2'; \
+	$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
+	--log-file $$b.log --trs-file $$b.trs \
+	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
+	"$$tst" $(AM_TESTS_FD_REDIRECT)
+package_metadata_test2b.log: package_metadata_test2b$(EXEEXT)
+	@p='package_metadata_test2b$(EXEEXT)'; \
+	b='package_metadata_test2b'; \
 	$(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
 	--log-file $$b.log --trs-file $$b.trs \
 	$(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
@@ -10521,9 +10571,21 @@  uninstall-am:
 @DEFAULT_TARGET_X86_64_TRUE@	$(TEST_AS) -o $@ $<
 package_metadata_test.o: package_metadata_main.c
 	$(COMPILE) -c -o $@ $<
-package_metadata_test$(EXEEXT): package_metadata_test.o gcctestdir/ld
+package_metadata_test1$(EXEEXT): package_metadata_test.o gcctestdir/ld
 	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='{"foo":"bar"}'
 	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
+package_metadata_test1b$(EXEEXT): package_metadata_test.o gcctestdir/ld
+	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata=%7B%22foo%22%3A%22bar%22%7D
+	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
+package_metadata_test1c$(EXEEXT): package_metadata_test.o gcctestdir/ld
+	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='%[lbrace]%[quot]foo%[quot]:%[quot]bar%[quot]%[rbrace]'
+	$(TEST_READELF) --notes $@ | grep -q '{"foo":"bar"}'
+package_metadata_test2$(EXEEXT): package_metadata_test.o gcctestdir/ld
+	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata=%7B%22name%22:%22binutils%22%2C%22ver%22%3A%22x%20%%22%7d
+	$(TEST_READELF) --notes $@ | grep -q '{"name":"binutils","ver":"x %"}'
+package_metadata_test2b$(EXEEXT): package_metadata_test.o gcctestdir/ld
+	$(CXXLINK) package_metadata_test.o -Wl,--package-metadata='{%[quot]name%[quot]:%[quot]binutils%[quot]%[comma]%[quot]ver%[quot]:%[quot]x%[space]%%[quot]}'
+	$(TEST_READELF) --notes $@ | grep -q '{"name":"binutils","ver":"x %"}'
 
 # Tell versions [3.59,3.63) of GNU make to not export all variables.
 # Otherwise a system limit (for SysV at least) may be exceeded.