ping for [PATCH] gold: Support percent-encoded JSON in --package-metadata
Benjamin Drung
benjamin.drung@canonical.com
Tue Dec 17 12:09:39 GMT 2024
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.
More information about the Binutils
mailing list