[PATCH] ld: Add --encoded-package-metadata

[Benjamin Drung]

On Tue, 2024-07-16 at 08:19 +0200, Jan Beulich wrote:
> On 15.07.2024 20:43, Benjamin Drung wrote:
> > Specifying the compiler flag `-Wl,--package-metadata=<JSON>` might not
> > work, because the shells might eat the quotation marks and the compiler
> > splits the JSON at the commas.
> > 
> > Ubuntu tried to using a specs file to set `--package-metadata` but that
> > turned out to be too fragile. autopkgtests might use the compiler flags
> > but the needed environment variables are not set in the test
> > environment. Debugging a crash of an application build with the -spec
> > parameter lacks the environment variables. People like to iteratively
> > continue building the software in the build directory while hacking on
> > the package and then have no environment variable set.
> > 
> > So introduce a `--encoded-package-metadata` linker flag that takes a
> > percent-encoded JSON. Percent-encoding is used because it is a
> > standard and simple to implement.
> > 
> > Bug-Ubutru: https://bugs.launchpad.net/bugs/2071468
> > Signed-off-by: Benjamin Drung <benjamin.drung@canonical.com>
> 
> Fundamentally fine, but please add a testcase (even if it's a contrived one).
> And there are a number of style issues (see below).
> 

I am working on a test case, but I'll send the reformatted code as patch
v2 for allowing to review the formatting changes. While writing the
code, I tried to follow what the existing code did, but failed horribly.

> > --- a/ld/emultempl/elf.em
> > +++ b/ld/emultempl/elf.em
> > @@ -813,6 +813,7 @@ EOF
> >  fi
> >  fragment <<EOF
> >      {"build-id", optional_argument, NULL, OPTION_BUILD_ID},
> > +    {"encoded-package-metadata", optional_argument, NULL, OPTION_ENCODED_PACKAGE_METADATA},
> >      {"package-metadata", optional_argument, NULL, OPTION_PACKAGE_METADATA},
> >      {"compress-debug-sections", required_argument, NULL, OPTION_COMPRESS_DEBUG},
> >      {"rosegment", no_argument, NULL, OPTION_ROSEGMENT},
> > @@ -864,6 +865,23 @@ gld${EMULATION_NAME}_handle_option (int optc)
> >  	ldelf_emit_note_gnu_build_id = xstrdup (optarg);
> >        break;
> >  
> > +    case OPTION_ENCODED_PACKAGE_METADATA:
> > +      free ((char *) ldelf_emit_note_fdo_package_metadata);
> 
> This is, I think, the common GNU way of writing casts. I wouldn't insist on
> the blank after the closing paren here if it was missing, yet further down
> it is. My minimal request is: Please be consistent within a patch.
> 

That code line is used identical some lines below.

> > +      ldelf_emit_note_fdo_package_metadata = NULL;
> > +      if (optarg != NULL) {
> 
> Opening brace placement is wrong thoughout the entire patch.
> 

Hopefully fixed everywhere.

> > +	size_t len = strlen(optarg);
> 
> Blank missing ahead of the opening paren (also elsewhere).
> 
> > +	if (len > 0) {
> > +	  ldelf_emit_note_fdo_package_metadata = xmalloc (len + 1);
> > +	  int len = percent_decode (optarg,
> > +	                           ldelf_emit_note_fdo_package_metadata);
> > +	  if (len < 0) {
> > +	    einfo (_ ("%F%P: Failed to decode percent-encoded package metadata"
> > +	          " \`%s'\n"), optarg);
> 
> Yes, there are three examples of "_ (" in ld sources. The canonical spelling
> is "_(" though, unlike function calls.
> 
> > --- a/ld/ldmisc.c
> > +++ b/ld/ldmisc.c
> > @@ -770,3 +770,53 @@ ld_abort (const char *file, int line, const char *fn)
> >    einfo (_("%F%P: please report this bug\n"));
> >    xexit (1);
> >  }
> > +
> > +/* Decode a hexadecimal character. Return -1 on error. */
> > +static int
> > +hexdecode(char c) {
> > +  if ('0' <= c && c <= '9') {
> > +    return c - '0';
> > +  }
> > +  if ('A' <= c && c <= 'F') {
> > +    return c - 'A' + 10;
> > +  }
> > +  if ('a' <= c && c <= 'f') {
> > +    return c - 'a' + 10;
> > +  }
> > +  return -1;
> > +}
> > +
> > +/* Decode a percent-encoded string. dst must be at least the same size as src.
> > +   It can be converted in place. Returns the lenght of the decoded string
> > +   (without training null character) or -1 on error. */
> > +int
> > +percent_decode(char *src, char *dst) {
> 
> "const char *" for "src" please.
> 

Changed.

> > +  int length = 0;
> > +  while (*src != '\0') {
> > +    char c = *src++;
> > +    if (c != '%') {
> > +      *dst++ = c;
> > +      length += 1;
> > +      continue;
> > +    }
> > +    char next1 = *src++;
> > +    if (next1 == '%') {
> > +      // Encoded %
> > +      *dst++ = c;
> > +      length += 1;
> > +      continue;
> > +    }
> > +    int hex1 = hexdecode(next1);
> > +    if (hex1 == -1) {
> > +      return -1;
> > +    }
> > +    int hex2 = hexdecode(*src++);
> > +    if (hex2 == -1) {
> > +      return -1;
> > +    }
> > +    *dst++ = ((char)hex1 << 4) + (char)hex2;
> 
> I'm curious: What use are the casts here? Especially the left one does,
> afaict, absolutely nothing: hex1 is converted to char (without changing the
> value, given what hexdecode() can return) just to then be promoted back to
> int.

I changed it to do the char conversion after the full calculation.

-- 
Benjamin Drung
Debian & Ubuntu Developer