Re: [PATCH 1/3] Use BUILD_PATH_PREFIX_MAP envvar for debug-prefix-map

[Jeff Law <law at redhat dot com>]

On 07/21/2017 10:15 AM, Ximin Luo wrote:
> Define the BUILD_PATH_PREFIX_MAP environment variable, and treat it as implicit
> -fdebug-prefix-map CLI options specified before any explicit such options.
> 
> Much of the generic code for applying and parsing prefix-maps is implemented in
> libiberty instead of the dwarf2 parts of the code, in order to make subsequent
> patches unrelated to debuginfo easier.
> 
> Acknowledgements
> ----------------
> 
> Daniel Kahn Gillmor who wrote the patch for r231835, which saved me a lot of
> time figuring out what to edit.
> 
> HW42 for discussion on the details of the proposal, and for suggesting that we
> retain the ability to map the prefix to something other than ".".
> 
> Other contributors to the BUILD_PATH_PREFIX_MAP specification, see
> https://reproducible-builds.org/specs/build-path-prefix-map/
> 
> ChangeLogs
> ----------
> 
> include/ChangeLog:
> 
> 2017-07-21  Ximin Luo  <infinity0@pwned.gg>
> 
> 	* prefix-map.h: New file implementing the BUILD_PATH_PREFIX_MAP
> 	specification; includes code from /gcc/final.c and code adapted from
> 	examples attached to the specification.
> 
> libiberty/ChangeLog:
> 
> 2017-07-21  Ximin Luo  <infinity0@pwned.gg>
> 
> 	* prefix-map.c: New file implementing the BUILD_PATH_PREFIX_MAP
> 	specification; includes code from /gcc/final.c and code adapted from
> 	examples attached to the specification.
> 	* Makefile.in: Update for new files.
> 
> gcc/ChangeLog:
> 
> 2017-07-21  Ximin Luo  <infinity0@pwned.gg>
> 
> 	* debug.h: Declare add_debug_prefix_map_from_envvar.
> 	* final.c: Define add_debug_prefix_map_from_envvar, and refactor
> 	prefix-map utilities to use equivalent code from libiberty instead.
> 	* opts-global.c: (handle_common_deferred_options): Call
> 	add_debug_prefix_map_from_envvar before processing options.
> 
> gcc/testsuite/ChangeLog:
> 
> 2017-07-21  Ximin Luo  <infinity0@pwned.gg>
> 
> 	* lib/gcc-dg.exp: Allow dg-set-compiler-env-var to take only one
> 	argument in which case it unsets the given env var.
> 	* gcc.dg/debug/dwarf2/build_path_prefix_map-1.c: New test.
> 	* gcc.dg/debug/dwarf2/build_path_prefix_map-2.c: New test.
I think some revamping will be needed here to avoid using the
environment variable.

Conceptually I'm OK with doing much of the work in libiberty, driven by GCC.



> 
> Index: gcc-8-20170716/include/prefix-map.h
> ===================================================================
> --- /dev/null
> +++ gcc-8-20170716/include/prefix-map.h
> @@ -0,0 +1,94 @@
> +/* Declarations for manipulating filename prefixes.
> +   Written 2017 by Ximin Luo <infinity0@pwned.gg>
> +   This code is in the public domain. */
> +
> +#ifndef _PREFIX_MAP_H
> +#define _PREFIX_MAP_H
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#ifdef HAVE_STDLIB_H
> +#include <stdlib.h>
> +#endif
> +
> +/* Linked-list of mappings from old prefixes to new prefixes.  */
Going to assume using a linked list for this stuff isn't a performance
concern in practice.  We shouldn't have enough prefixes for it to matter.


> +
> +struct prefix_map
> +{
> +  const char *old_prefix;
> +  const char *new_prefix;
> +  size_t old_len;
> +  size_t new_len;
> +  struct prefix_map *next;
> +};
> +
> +
> +/* Find a mapping suitable for the given OLD_NAME in the linked list MAP.\
> +
> +   If a mapping is found, writes a pointer to the non-matching suffix part of
> +   OLD_NAME in SUFFIX, and its length in SUF_LEN.
> +
> +   Returns NULL if there was no suitable mapping.  */
> +struct prefix_map *
> +prefix_map_find (struct prefix_map *map, const char *old_name,
> +		 const char **suffix, size_t *suf_len);
> +
> +/* Prepend a prefix map before a given SUFFIX.
> +
> +   The remapped name is written to NEW_NAME and returned as a const pointer. No
> +   allocations are performed; the caller must ensure it can hold at least
> +   MAP->NEW_LEN + SUF_LEN + 1 characters.  */
> +const char *
> +prefix_map_prepend (struct prefix_map *map, char *new_name,
> +		    const char *suffix, size_t suf_len);
> +
> +/* Remap a filename.
> +
> +   Returns OLD_NAME unchanged if there was no remapping, otherwise returns a
> +   pointer to newly-allocated memory for the remapped filename.  The memory is
> +   allocated by the given ALLOC function, which also determines who is
> +   responsible for freeing it.  */
> +#define prefix_map_remap_alloc_(map_head, old_name, alloc)		       \
> +  __extension__								       \
> +  ({									       \
> +    const char *__suffix;						       \
> +    size_t __suf_len;							       \
> +    struct prefix_map *__map;						       \
> +    (__map = prefix_map_find ((map_head), (old_name), &__suffix, &__suf_len))  \
> +      ? prefix_map_prepend (__map,					       \
> +			    (char *) alloc (__map->new_len + __suf_len + 1),   \
> +			    __suffix, __suf_len)			       \
> +      : (old_name);							       \
> +  })
Is there some significant value in using the statements as expressions
extension here?  If not, just make this a function.

> +
> +/* Remap a filename.
> +
> +   Returns OLD_NAME unchanged if there was no remapping, otherwise returns a
> +   stack-allocated pointer to the newly-remapped filename.  */
> +#define prefix_map_remap_alloca(map_head, old_name) \
> +  prefix_map_remap_alloc_ (map_head, old_name, alloca)
Seems unwise to #define something like this is the normal namespace
within a header file.  At the least you should #undef it so that it
doesn't bleed out.   But again, why not just make this a function?



> Index: gcc-8-20170716/libiberty/prefix-map.c
> ===================================================================
> --- /dev/null
> +++ gcc-8-20170716/libiberty/prefix-map.c


> Index: gcc-8-20170716/gcc/debug.h
> ===================================================================
> --- gcc-8-20170716.orig/gcc/debug.h
> +++ gcc-8-20170716/gcc/debug.h
> @@ -236,6 +236,7 @@ extern void dwarf2out_switch_text_sectio
>  
>  const char *remap_debug_filename (const char *);
>  void add_debug_prefix_map (const char *);
> +void add_debug_prefix_map_from_envvar ();
>  
>  /* For -fdump-go-spec.  */
>  
> Index: gcc-8-20170716/gcc/final.c
> ===================================================================
> --- gcc-8-20170716.orig/gcc/final.c
> +++ gcc-8-20170716/gcc/final.c
> @@ -46,6 +46,7 @@ along with GCC; see the file COPYING3.
>  #define INCLUDE_ALGORITHM /* reverse */
>  #include "system.h"
>  #include "coretypes.h"
> +#include "prefix-map.h"
>  #include "backend.h"
>  #include "target.h"
>  #include "rtl.h"
> @@ -1506,22 +1507,9 @@ asm_str_count (const char *templ)
>    return count;
>  }
>  
> -/* ??? This is probably the wrong place for these.  */
> -/* Structure recording the mapping from source file and directory
> -   names at compile time to those to be embedded in debug
> -   information.  */
> -struct debug_prefix_map
> -{
> -  const char *old_prefix;
> -  const char *new_prefix;
> -  size_t old_len;
> -  size_t new_len;
> -  struct debug_prefix_map *next;
> -};
> -
> -/* Linked list of such structures.  */
> -static debug_prefix_map *debug_prefix_maps;
>  
> +/* Linked list of `struct prefix_map'.  */
> +static prefix_map *debug_prefix_maps = NULL;
>  
>  /* Record a debug file prefix mapping.  ARG is the argument to
>     -fdebug-prefix-map and must be of the form OLD=NEW.  */
> @@ -1529,7 +1517,7 @@ static debug_prefix_map *debug_prefix_ma
>  void
>  add_debug_prefix_map (const char *arg)
>  {
> -  debug_prefix_map *map;
> +  prefix_map *map;
>    const char *p;
>  
>    p = strchr (arg, '=');
> @@ -1538,7 +1526,7 @@ add_debug_prefix_map (const char *arg)
>        error ("invalid argument %qs to -fdebug-prefix-map", arg);
>        return;
>      }
> -  map = XNEW (debug_prefix_map);
> +  map = XNEW (prefix_map);
>    map->old_prefix = xstrndup (arg, p - arg);
>    map->old_len = p - arg;
>    p++;
> @@ -1548,28 +1536,32 @@ add_debug_prefix_map (const char *arg)
>    debug_prefix_maps = map;
>  }
>  
> +/* Add debug-prefix-maps from BUILD_PATH_PREFIX_MAP environment variable.  */
> +
> +void
> +add_debug_prefix_map_from_envvar ()
> +{
> +  const char *arg = getenv ("BUILD_PATH_PREFIX_MAP");
> +
> +  if (!arg || prefix_map_parse (&debug_prefix_maps, arg))
> +    return;
> +
> +  error ("environment variable BUILD_PATH_PREFIX_MAP is "
> +	 "not well formed; see the GCC documentation for more details.");
> +}
> +
>  /* Perform user-specified mapping of debug filename prefixes.  Return
>     the new name corresponding to FILENAME.  */
>  
>  const char *
>  remap_debug_filename (const char *filename)
>  {
> -  debug_prefix_map *map;
> -  char *s;
> -  const char *name;
> -  size_t name_len;
> -
> -  for (map = debug_prefix_maps; map; map = map->next)
> -    if (filename_ncmp (filename, map->old_prefix, map->old_len) == 0)
> -      break;
> -  if (!map)
> +  const char *name = prefix_map_remap_alloca (debug_prefix_maps, filename);
alloca?  Let's avoid alloca if reasonably possible.  Particularly if the
size of the alloca'd area could potentially be under user control.


jeff