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>, gdb-patches at sourceware dot org
- Date: Tue, 11 Nov 2014 18:53:26 +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>
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