This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v3 14/14] the "compile" command
- From: Pedro Alves <palves at redhat dot com>
- To: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Fri, 12 Dec 2014 14:38:12 +0000
- Subject: Re: [PATCH v3 14/14] the "compile" command
- Authentication-results: sourceware.org; auth=none
- References: <20141101214552 dot 13230 dot 45564 dot stgit at host1 dot jankratochvil dot net> <20141101214733 dot 13230 dot 49968 dot stgit at host1 dot jankratochvil dot net> <54625B26 dot 8000309 at redhat dot com> <20141123183644 dot GA31165 at host2 dot jankratochvil dot net>
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