Re: [PATCH] Add support for global-file-syms to the clang and llvm plugins

[Nick Clifton <nickc@redhat.com>]

Hi Tulio,

> This part starts by moving the implementation of parse_env() to
> annobin-common.cc in order to be shared between the gcc and the llvm
> plugins.
> 
> This was required because the llvm plugin didn't have a mechanism to
> receive arguments from the user. This commit adds support for using the
> environment variable ANNOBIN for the LLVM plugin, although it's still
> missing many of the basic features available in the other plugins.
> 
> Both clang and llvm plugins are now able to generate the same output as
> the GCC plugin, e.g. _annobin_hello_c_1711138217_00204218_start.

Thanks very much for writing this patch.

I do have a few comments on the code, but overall I thought that it was
excellent.

> diff --git a/annobin-common.cc b/annobin-common.cc

> +extern bool parse_argument (const char *, const char *);

There is no external function called parse_argument() so you do not
have to provide a prototype for it...

> +void
> +parse_env (bool (* parse_argument)(const char *, const char *))

I think that the function should be called 'annobin_parse_env' rather
than 'parse_env'.  There are two reasons: 1) there is a distinct
possibility that it might clash with a same-named function in the
sources of Clang, LLVM or GCC one day.  2) when running the compiler
inside a debugger it is helpful to have function names that indicate
to which part of the program they belong.  Thus a user using gdb to
debug a clang crash for example might run across "parse_env" and have
no idea where to look for it in the sources.  But if they see
"annobin_parse_env" they immediately have a big clue for where to look.

Also - the function should return a BOOL based upon the results
of calling parse_argument().  Ie if any invocation of parse_argument()
fails, then parse_env() should also fail.

Also - the parse_argument function should take a third argument, a void *
pointer provided to parse_env() by the caller.  The reason for this is
that the caller may wish to pass extra information to the parse_argument
function.


> +{
> +  char arg[2048];

Large buffers on the stack are a security hazzard.  Better to make
the buffer static - or better yet, dynamic.

> +      comma = strchr (env, ',');
> +      if (comma)
> +	{
> +	  strncpy (arg, env, comma - env);

Potential buffer overflow if (common - env >= sizeof arg)


> diff --git a/annobin-common.h b/annobin-common.h

> +void parse_env (bool (* parse_argument)(const char *, const char *));
> +#endif // ANNOBIN-COMMON_H_

I am a firm believer in header files providing descriptions of the functions
that they prototype.  This makes it easier for users unfamiliar with the source
code to use the header without having to delve into the sources themselves.
So in the version of your patch that I have checked in I added:

/* Checks for an ANNOBIN environment variable.
    If it exists, parses it for comma separated command line options,
     passing each in turn to PARSE_ARGUMENT.  Arguments are copied
     into a buffer before processing and can be manipulated by
     PARSE_ARGUMENT.  If an argument contains a '=' character then
     it is split into two pieces and passed to PARSE_ARGUMENT as
     (NAME, VALUE).  Otherwise it is just passed as (NAME, "").
     The return value from PARSE_ARGUMENT is recorded, but
     parsing will continue even if PARSE_ARGUMENT returns FALSE.
    Returns TRUE if the ANNOBIN environment variable does not exist.
    Returns TRUE if the ANNOBIN environment exists and PARSE_ARGUMENT
     returns TRUE for all of the arguments.
    Return FALSE otherwise.  */


I also thought that we should be consistent and allow the clang plugin to also
parse the ANNOBIN environment variable.  So I added that.  But basically I
was very happy with your patch and I have now checked it in to the annobin
repository.

Cheers
   Nick

PS. I have not built the rawhide version yet as I have run into some problems with the testsuites...