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: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- To: Pedro Alves <palves at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Sun, 23 Nov 2014 19:36:44 +0100
- 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>
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