Re: [PATCH v2 3/4] Add DWARF index cache

[Eli Zaretskii <eliz@gnu.org>]

> From: Simon Marchi <simon.marchi@ericsson.com>
> CC: Simon Marchi <simon.marchi@ericsson.com>
> Date: Wed, 25 Jul 2018 18:47:03 -0400
> 
> GDB can generate indexes for DWARF debug information, which, when
> integrated in the original binary, can speed up loading object files.
> This can be done using the gdb-add-index script or directly by the
> linker itself.

The information in the last sentence is not mentioned in the
documentation patch you provided.  I think we should mention that.

> To help make using the DWARF index more transparent, this patch
> introduces a DWARF index cache.  When enabled, loading an index-less
> binary in GDB will automatically save an index file in ~/.cache/gdb.
> When loading that same object file again, the index file will be looked
> up and used to load the DWARF index.  You therefore get the benefit of
> the DWARF index without having to do additional manual steps or
> modifying your build system.  When an index section is already present
> in the file, GDB will prefer that one over looking up the cache.

Does GDB check whether the cache became stale, i.e. the program was
modified since the last save?  This should be documented.  And if it
does check, what happens if the cache is outdated? are there any user
level messages?

> - The saved index file is exactly the same as the output of the "save
>   gdb-index" command.  It is therefore the exact same content that would
>   be found in the .gdb_index or .debug_names section.  We just leave it
>   as a standalone file instead of merging it in the binary.

This should be mentioned in the documentation, IMO.

> - The code was made to follow the XDG specification: if the
>   XDG_CACHE_HOME environment variable, it is used, otherwise it falls
>   back to ~/.cache/gdb.  It is possible to change it using "set
>   index-cache directory".  On other OSes than GNU/Linux, ~/.cache may
>   not be the best place to put such data.  On macOS it should probably
>   default to ~/Library/Caches/...  On Windows, %LocalAppData%/...  I
>   don't intend to do this part, but further patches are welcome.

IMO, we should only use LocalAppData on Windows if we already do that
for .gdbinit etc.  (And the platform standards on Windows actually
frown on putting files into that directory; you are supposed to put
them in an application-specific subdirectory.)

> - I think that we need to be careful that multiple instances of GDB
>   don't interfere with each other (not far fetched at all if you run GDB
>   in some automated script) and the cache is always coherent (either the
>   file is not found, or it is found and entirely valid).  Writing the
>   file directly to its final location seems like a recipe for failure.
>   One GDB could read a file in the index while it is being written by
>   another GDB.  To mitigate this, I made write_psymtabs_to_index write
>   to temporary files and rename them once it's done.  Two GDB instances
>   writing the index for the same file should not step on each other's
>   toes (the last file to be renamed will stay).  A GDB looking up a file
>   will only see a complete file or no file.  Also, if GDB crashes while
>   generating the index file, it will leave a work-in-progress file, but
>   it won't be picked up by other instances looking up in the cache.

Maybe we should consider some kind of interlocking, like Emacs does.

> +  while (1)
> +    {
> +      /* Find the beginning of the next component.  */
> +      while (*component_start == '/')
> +	component_start++;
> +
> +      /* Are we done?  */
> +      if (*component_start == '\0')
> +	return;
> +
> +      /* Find the slash or null-terminator after this component.  */
> +      component_end = component_start;
> +      while (*component_end != '/' && *component_end != '\0')
> +	component_end++;

I believe literal uses of '/' are non-portable; you should use
IS_DIR_SEPARATOR (defined in filenames.h) instead.  You also cannot
portably assume an absolute file name always starts with a slash.

> +      /* If we get EEXIST and the existing path is a directory, then we're
> +         happy.  If it exists, but it's a regular file and this is not the last
> +         component, we'll fail at the next component.  If this is the last
> +         component, the caller will fail with ENOTDIR when trying to
> +         open/create a file under that path.  */
> +      if (mkdir (start, 0700) != 0)

This will not work on Windows.

> +	if (errno != EEXIST)
> +	  return;

I think on Windows the value of errno is different.

> +  std::string filename = make_index_filename (build_id, INDEX4_SUFFIX);
> +
> +  TRY
> +    {
> +      if (debug_index_cache)
> +        printf_unfiltered ("index cache: trying to read %s\n",
> +			   filename.c_str ());
> +
> +      /* Try to map that file.  */
> +      index_cache_resource_mmap *mmap_resource
> +	= new index_cache_resource_mmap (filename.c_str ());
> +
> +      /* Yay, it worked!  Hand the resource to the caller.  */
> +      resource->reset (mmap_resource);
> +
> +      return gdb::array_view<const gdb_byte>
> +	  ((const gdb_byte *) mmap_resource->mapping.get (),
> +	   mmap_resource->mapping.size ());
> +    }
> +  CATCH (except, RETURN_MASK_ERROR)
> +    {
> +      if (debug_index_cache)
> +	printf_unfiltered ("index cache: couldn't read %s: %s\n",
> +			 filename.c_str (), except.message);
> +    }
> +  END_CATCH

Does this mean this feature will only work on systems with mmap?

Thanks.