Re: [PATCH v3 14/14] the "compile" command

[Pedro Alves <palves@redhat.com>]

Hi Jan,

FWIW, this looks good to me, and I'm looking forward to
have this pushed in.  Just a few comments below.

> +/* See compile-internal.h.  */
> +
> +void
> +gcc_convert_symbol (void *datum,
> +		    struct gcc_c_context *gcc_context,
> +		    enum gcc_c_oracle_request request,
> +		    const char *identifier)

...

> +
> +  /* We can't allow exceptions to escape out of this callback.  Safest
> +     is to simply emit a gcc error.  */
> +  TRY_CATCH (e, RETURN_MASK_ERROR)
> +    {
> +      struct symbol *sym;

Shouldn't this catch ctrl-c too then?  Likewise the other hooks.





> +      for (i = 0; i < 4; ++i)

It'd be good to give this magic constant a name, or at
a least a comment.

> +	{
> +	  const char *mode = c_get_mode_for_size (1 << i);
> +
> +	  gdb_assert (mode != NULL);
> +	  fprintf_unfiltered (buf,
> +			      "typedef int"
> +			      " __attribute__ ((__mode__(__%s__)))"
> +			      " __gdb_int_%s;\n",
> +			      mode, mode);
> +	}





> +
> +/* A cleanup function to remove a directory and all its contents.  */
> +
> +static void
> +do_rmdir (void *arg)
> +{
> +  char *zap = concat ("rm -rf ", arg, (char *) NULL);
> +
> +  system (zap);
> +}

This is quite scary...  Could we please add an assert here
that tempdir_name starts with "/tmp/gdbobj-", just in case something
goes really wrong here?

> +
> +/* Return the name of the temporary directory to use for .o files, and
> +   arrange for the directory to be removed at shutdown.  */
> +
> +static const char *
> +get_compile_file_tempdir (void)
> +{
> +  static char *tempdir_name;
> +
> +#define TEMPLATE "/tmp/gdbobj-XXXXXX"
> +  char tname[sizeof (TEMPLATE)];
> +
> +  if (tempdir_name != NULL)
> +    return tempdir_name;
> +
> +  strcpy (tname, TEMPLATE);
> +#undef TEMPLATE
> +  tempdir_name = mkdtemp (tname);
> +  if (tempdir_name == NULL)
> +    perror_with_name (_("Could not make temporary directory"));
> +
> +  tempdir_name = xstrdup (tempdir_name);
> +  make_final_cleanup (do_rmdir, tempdir_name);
> +  return tempdir_name;
> +}




> +  /* Override flags possibly coming from DW_AT_producer.  */
> +  compile_args = xstrdup ("-O0 -gdwarf-4"
> +  /* We use -fPIC to ensure that we can reference properly.  Otherwise
> +     on x86-64 a string constant's address might be truncated when gdb
> +     loads the object; another approach would be -mcmodel=large, but
> +     -fPIC seems more portable across back ends.  */

This comment ends up being a bit unexpected/odd, given that ...

> +			 " -fPIC"
> +  /* We don't want warnings.  */
> +			 " -w"

... patch #6 has:

> +char *
> +default_gcc_target_options (struct gdbarch *gdbarch)
> +{
> +  return xstrprintf ("-m%d%s", gdbarch_ptr_bit (gdbarch),
> +		     gdbarch_ptr_bit (gdbarch) == 64 ? " -mcmodel=large" : "");
> +}
> +

IOW, is the comment stale?

Thanks,
Pedro Alves