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