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

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Tue, 11 Nov 2014 19:53:26 +0100, Pedro Alves wrote:
> > +/* 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.

Yes, changed.


> > +      for (i = 0; i < 4; ++i)
> 
> It'd be good to give this magic constant a name, or at
> a least a comment.

Added there:
	// Iterate all log2 sizes in bytes supported by c_get_mode_for_size.
Added to c_get_mode_for_size:
	default:
	  internal_error (__FILE__, __LINE__, _("Invalid GCC mode size %d."), size);


> > +	{
> > +	  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?

Added:
	/* Initial filename for temporary files.  */

	#define TMP_PREFIX "/tmp/gdbobj-"
+
	#define TEMPLATE TMP_PREFIX "XXXXXX"
+
	gdb_assert (strncmp (dir, TMP_PREFIX, strlen (TMP_PREFIX)) == 0);


> > +  /* 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?

You are right the comment is stale although the code is not stale.  Up to date
reason for these options I have written in:
	https://sourceware.org/gdb/wiki/GCCCompileAndExecute#Relocating_the_object_file
Therefore put there:
	  /* We use -fPIC Otherwise GDB would need to reserve space large enough for
	     any object file in the inferior in advance to get the final address when
	     to link the object file to and additionally the default system linker
	     script would need to be modified so that one can specify there the
	     absolute target address.  */
				" -fPIC"

And also:
	/* -mcmodel=large is used so that no GOT (Global Offset Table) is needed to be
	   created in inferior memory by GDB (normally it is set by ld.so).  */

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


Thanks,
Jan