Playing with GCC plugins

Martin Kletzander’s devconf.cz talk about features in modern compilers that we probably don’t use inspired me to play with GCC plugins. I want to see if I can use a plugin to solve a horrible bug we hit at the end of last year.

We had a C struct defined in a header file like this:

struct {
  int foo;
#ifdef HAVE_LIBVIRT
  int bar;
#endif
  int baz;
};

But because of the way HAVE_LIBVIRT is defined, if you used the header file as:

#include <config.h>
#include "mystruct.h"

the struct would be defined with the bar element. But if you used the struct like this:

#include "mystruct.h"

you didn’t get the bar element. Ooops. This silently caused data corruption (when accessing the third member of the struct).

My idea is that we could use a GCC plugin to print out the size of every struct we define when compiling the code, and then you can post-process that to see if there are structs which unexpectedly change size.

The GCC plugin API is not exactly well documented. In fact, without reading a lot of GCC header files and internals, it’s virtually impossible to work out what is going on. Anyway, my plugin below works for me on GCC 6.

You have to compile the plugin using:

gcc -g -I`gcc -print-file-name=plugin`/include \
    -fpic -shared -o structsizes.so structsizes.cc

and then when you compile your source code you have to add:

gcc -fplugin=./structsizes.so \
    -fplugin-arg-structsizes-log=<logfile> ...

The plugin writes simple text records to logfile:

struct 'random_data' has size 384 [bits]
struct 'drand48_data' has size 192 [bits]
...

which you can easily post-process with some shell script. Note you should compile your source with make -j1 so that parallel runs of the compiler don’t try to write to the log file at the same time, since my plugin doesn’t include any locking.

/* structsizes.cc plugin: public domain example code written by
 * Richard W.M. Jones
 */

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <gcc-plugin.h>
#include <tree.h>
#include <print-tree.h>

int plugin_is_GPL_compatible;

static FILE *log_fp;

static void
plugin_finish_type (void *event_data, void *user_data)
{
  tree type = (tree) event_data;

  /* We only care about structs, not any other type definition. */
  if (TREE_CODE (type) == RECORD_TYPE) {
#if 0
    /* This is useful for working out how to navigate the tree below. */
    debug_tree (type);
#endif

    /* Struct name? */
    tree name_tree = TYPE_NAME (type);

    /* Ignore unnamed structs. */
    if (!name_tree) {
      fprintf (log_fp, "ignoring unnamed struct\n");
      return;
    }

    const char *name;
    if (TREE_CODE (name_tree) == IDENTIFIER_NODE)
      name = IDENTIFIER_POINTER (name_tree);
    else if (TREE_CODE (name_tree) == TYPE_DECL && DECL_NAME (name_tree))
      name = IDENTIFIER_POINTER (DECL_NAME (name_tree));
    else
      name = "unknown struct name"; /* should never happen? */

    /* If the type is not complete, we can't do anything. */
    if (!COMPLETE_TYPE_P (type)) {
      fprintf (log_fp, "struct '%s' has incomplete type\n", name);
      return;
    }

    /* Get the size of the struct that has been defined. */
    tree size_tree = TYPE_SIZE (type);
    if (TREE_CODE (size_tree) == INTEGER_CST &&
        !TYPE_P (size_tree) && TREE_CONSTANT (size_tree)) {
      size_t size = TREE_INT_CST_LOW (size_tree);
      fprintf (log_fp, "struct '%s' has size %zu [bits]\n", name, size);
    }
    else
      fprintf (log_fp, "struct '%s' has non-constant size\n", name);
  }

  fflush (log_fp);
}

int
plugin_init (struct plugin_name_args *plugin_info,
             struct plugin_gcc_version *version)
{
  const char *logfile = NULL;
  size_t i;

  /* Open the log file. */
  for (i = 0; i < plugin_info->argc; ++i) {
    if (strcmp (plugin_info->argv[i].key, "log") == 0) {
      logfile = plugin_info->argv[i].value;
    }
  }

  if (!logfile) {
    fprintf (stderr, "structsizes plugin: missing parameter: -fplugin-arg-structsizes-log=<logfile>\n");
    exit (EXIT_FAILURE);
  }

  log_fp = fopen (logfile, "a");
  if (log_fp == NULL) {
    perror (logfile);
    exit (EXIT_FAILURE);
  }

  fprintf (log_fp, "Loaded structsizes plugin (GCC %s.%s.%s)\n",
           version->basever, version->devphase, version->revision);

  register_callback (plugin_info->base_name, PLUGIN_FINISH_TYPE,
                     plugin_finish_type, NULL);

  return 0;
}

Using __attribute__((cleanup)) in libguestfs

After some discussion upstream (mostly on IRC I’m sad to say), I have pushed a patch to use GCC’s __attribute__((cleanup)) extension (also supported by LLVM, but not by anything else).

This attribute causes a destructor to run automatically when a variable goes out of scope. In libguestfs we’ve added some CLEANUP_* macros to make things a little bit simpler, so I’m going to use those macros in these examples:

{
  CLEANUP_FREE char *buf = NULL;
  ...
  if (asprintf (&buf, "%s/%s", tmpdir, filename) == -1) {
    reply_with_perror ("asprintf");
    return -1;
  }
  ...
  return 0;
}

The CLEANUP_FREE macro ensures that the equivalent of free (buf) is called on every exit path from the scope where the variable ‘buf’ is defined, which means on every return, but also on other exits such as goto or falling off the end.

For most functions, this may save a line or two. Occasionally it can make functions a lot simpler. Compare before:

static char *
icon_windows_xp (guestfs_h *g, struct inspect_fs *fs, size_t *size_r)
{
  char *filename = NULL;
  char *filename_case = NULL;
  char *filename_downloaded = NULL;
  char *pngfile = NULL;
  char *ret;
  struct command *cmd;
  int r;

  /* Download %systemroot%\explorer.exe */
  filename = safe_asprintf (g, "%s/explorer.exe", fs->windows_systemroot);
  filename_case = guestfs___case_sensitive_path_silently (g, filename);
  if (filename_case == NULL)
    goto not_found;

  filename_downloaded = guestfs___download_to_tmp (g, fs, filename_case,
                                                   "explorer.exe",
                                                   MAX_WINDOWS_EXPLORER_SIZE);
  if (filename_downloaded == NULL)
    goto not_found;

  pngfile = safe_asprintf (g, "%s/windows-xp-icon.png", g->tmpdir);

/*...*/
  if (r == -1)
    goto error;
  if (!WIFEXITED (r) || WEXITSTATUS (r) != 0)
    goto not_found;

  if (read_whole_file (g, pngfile, &ret, size_r) == -1)
    goto error;

  free (filename);
  free (filename_case);
  free (filename_downloaded);
  free (pngfile);
  return ret;

 error:
  free (filename);
  free (filename_case);
  free (filename_downloaded);
  free (pngfile);
  return NULL;

 not_found:
  free (filename);
  free (filename_case);
  free (filename_downloaded);
  free (pngfile);
  return NOT_FOUND;
}

and after:

static char *
icon_windows_xp (guestfs_h *g, struct inspect_fs *fs, size_t *size_r)
{
  CLEANUP_FREE char *filename = NULL;
  CLEANUP_FREE char *filename_case = NULL;
  CLEANUP_FREE char *filename_downloaded = NULL;
  CLEANUP_FREE char *pngfile = NULL;
  char *ret;
  struct command *cmd;
  int r;

  /* Download %systemroot%\explorer.exe */
  filename = safe_asprintf (g, "%s/explorer.exe", fs->windows_systemroot);
  filename_case = guestfs___case_sensitive_path_silently (g, filename);
  if (filename_case == NULL)
    return NOT_FOUND;

  filename_downloaded = guestfs___download_to_tmp (g, fs, filename_case,
                                                   "explorer.exe",
                                                   MAX_WINDOWS_EXPLORER_SIZE);
  if (filename_downloaded == NULL)
    return NOT_FOUND;

  pngfile = safe_asprintf (g, "%s/windows-xp-icon.png", g->tmpdir);

/*...*/
  if (r == -1)
    return NULL;
  if (!WIFEXITED (r) || WEXITSTATUS (r) != 0)
    return NOT_FOUND;

  if (read_whole_file (g, pngfile, &ret, size_r) == -1)
    return NULL;

  return ret;
}

It’s not all good news though. It’s almost certainly less efficient: a function call is made on every exit path, even ones which you could prove are irrelevant.

It may make the code more obscure, particularly for people who aren’t used to this feature.

It won’t work at all on non-GCC non-LLVM compilers, although I don’t think that is relevant to libguestfs.

There is some scope for error, especially double-freeing or accidentally freeing buffers which are returned from the function.

Half-baked ideas: Include licensing info in linker sections

For more half-baked ideas, see my ideas tag.

I had this idea when we were discussing the complexity of tracing the licensing of gnulib modules in libraries. It’s quite easy with gnulib to accidentally include a GPL module in your library that would otherwise be LGPL, thus (in theory — I don’t want to go into the legal arguments) “infecting” your LGPL library.

Here’s the idea: In each source file (or gnulib module or whatever is a convenient “licensing unit”) include a linker section describing the license. When the final library or binary is compiled, you can trivially see whether it includes the wrong licenses or mixes licenses inappropriately.

Here is how you do it. In each source file, add a line like this:

/* This library is GPL 3+. */
static int lib1_gpl3p __attribute__((section ("LICENSES")));

When you statically link a program or library, use readelf or objdump to reveal the licenses of the libraries it was linked against:

$ objdump -j LICENSES -t prog

prog:     file format elf64-x86-64

SYMBOL TABLE:
00000000006008dc l d  LICENSES	0000000000000000 LICENSES
00000000006008dc l  O LICENSES	0000000000000004 main_gpl3p
00000000006008e0 l  O LICENSES	0000000000000004 lib1_gpl3p
00000000006008e4 l  O LICENSES	0000000000000004 lib2_lgpl2p

(Dynamic linking is a little bit different: you’d add the same information to the header, or have to write a small tool which chased through the dependencies from ldd enumerating the licenses).

Joy of C macros

I wonder if my character constant is too long?

cc1: warnings being treated as errors
inspect.c: In function 'add_fstab_entry':
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:3: error: passing argument 1 of 'strlen' makes pointer from integer without a cast
/usr/include/string.h:399:15: note: expected 'const char *' but argument is of type 'int'
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:3: error: passing argument 1 of 'strlen' makes pointer from integer without a cast
/usr/include/string.h:399:15: note: expected 'const char *' but argument is of type 'int'
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:3: error: passing argument 1 of 'strlen' makes pointer from integer without a cast
/usr/include/string.h:399:15: note: expected 'const char *' but argument is of type 'int'
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:3: error: passing argument 1 of 'strlen' makes pointer from integer without a cast
/usr/include/string.h:399:15: note: expected 'const char *' but argument is of type 'int'
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:3: error: passing argument 1 of 'strlen' makes pointer from integer without a cast
/usr/include/string.h:399:15: note: expected 'const char *' but argument is of type 'int'
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:3: error: passing argument 2 of '__builtin_strcmp' makes pointer from integer without a cast
inspect.c:847:3: note: expected 'const char *' but argument is of type 'int'
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:3: error: passing argument 2 of '__builtin_strcmp' makes pointer from integer without a cast
inspect.c:847:3: note: expected 'const char *' but argument is of type 'int'
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:3: error: passing argument 1 of 'strlen' makes pointer from integer without a cast
/usr/include/string.h:399:15: note: expected 'const char *' but argument is of type 'int'
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:3: error: passing argument 2 of '__builtin_strcmp' makes pointer from integer without a cast
inspect.c:847:3: note: expected 'const char *' but argument is of type 'int'
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:3: error: passing argument 2 of '__builtin_strcmp' makes pointer from integer without a cast
inspect.c:847:3: note: expected 'const char *' but argument is of type 'int'
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:36: error: character constant too long for its type
inspect.c:847:3: error: passing argument 1 of 'strlen' makes pointer from integer without a cast
/usr/include/string.h:399:15: note: expected 'const char *' but argument is of type 'int'
inspect.c:847:3: error: passing argument 2 of 'strncmp' makes pointer from integer without a cast
/usr/include/string.h:146:12: note: expected 'const char *' but argument is of type 'int'