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

[Pedro Alves <palves@redhat.com>]

On 11/23/2014 06:36 PM, Jan Kratochvil wrote:
> 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.  I'm happy with these changes.

Thanks,
Pedro Alves