[PATCH] gdb: sim: automatically pass down sysroot

[PATCH] gdb: sim: automatically pass down sysroot

[Mike Frysinger]

Since gdb sets up a nice sysroot path for us by default, automatically
pass it down to the sim target so it too gets a good default.  This does
not override anything the user explicitly specifies of course.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>

2011-05-26  Mike Frysinger  <vapier@gentoo.org>

	* remote-sim.c (gdbsim_open): Add the strlen of " --sysroot=" and
	gdb_sysroot to the "len" variable.  Append both to "arg_buf".
---
 gdb/remote-sim.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/gdb/remote-sim.c b/gdb/remote-sim.c
index bf4e0ee..918d5cb 100644
--- a/gdb/remote-sim.c
+++ b/gdb/remote-sim.c
@@ -684,6 +684,7 @@ gdbsim_open (char *args, int from_tty)
   len = (7 + 1			/* gdbsim */
 	 + strlen (" -E little")
 	 + strlen (" --architecture=xxxxxxxxxx")
+	 + strlen (" --sysroot=") + strlen (gdb_sysroot) +
 	 + (args ? strlen (args) : 0)
 	 + 50) /* slack */ ;
   arg_buf = (char *) alloca (len);
@@ -708,6 +709,9 @@ gdbsim_open (char *args, int from_tty)
       strcat (arg_buf, " --architecture=");
       strcat (arg_buf, selected_architecture_name ());
     }
+  /* Pass along gdb's concept of the sysroot.  */
+  strcat (arg_buf, " --sysroot=");
+  strcat (arg_buf, gdb_sysroot);
   /* finally, any explicit args */
   if (args)
     {
-- 
1.7.5.rc3

Re: [PATCH] gdb: sim: automatically pass down sysroot

[Marek Polacek]

On 05/26/2011 10:03 PM, Mike Frysinger wrote:
> +  /* Pass along gdb's concept of the sysroot.  */
> +  strcat (arg_buf, " --sysroot=");
> +  strcat (arg_buf, gdb_sysroot);

BTW, couldn't be those strcat()s replaced by, e.g., cheaper mempcpy()?

	Marek

Re: [PATCH] gdb: sim: automatically pass down sysroot

[Mike Frysinger]

On Thu, May 26, 2011 at 16:09, Marek Polacek wrote:
> On 05/26/2011 10:03 PM, Mike Frysinger wrote:
>> +  /* Pass along gdb's concept of the sysroot.  */
>> +  strcat (arg_buf, " --sysroot=");
>> +  strcat (arg_buf, gdb_sysroot);
>
> BTW, couldn't be those strcat()s replaced by, e.g., cheaper mempcpy()?

the whole func probably could be converted, but i was keeping existing style
-mike

Re: [PATCH] gdb: sim: automatically pass down sysroot

[Tom Tromey]

>>>>> "Marek" == Marek Polacek <mpolacek@redhat.com> writes:

Marek> BTW, couldn't be those strcat()s replaced by, e.g., cheaper mempcpy()?

Thanks for noticing this.

Normally my preference is to prefer the simplest code unless it is known
to be performance-critical.  In a case like this I doubt that it
matters.

Tom

Re: [PATCH] gdb: sim: automatically pass down sysroot

[Pedro Alves]

On Friday 27 May 2011 18:47:49, Tom Tromey wrote:
> >>>>> "Marek" == Marek Polacek <mpolacek@redhat.com> writes:
> 
> Marek> BTW, couldn't be those strcat()s replaced by, e.g., cheaper mempcpy()?
> 
> Thanks for noticing this.
> 
> Normally my preference is to prefer the simplest code unless it is known
> to be performance-critical.  In a case like this I doubt that it
> matters.

And mempcpy is not portable --- we'd have to pull it from gnulib.

-- 
Pedro Alves

Re: [PATCH] gdb: sim: automatically pass down sysroot

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 847 bytes --]

> 2011-05-26  Mike Frysinger  <vapier@gentoo.org>
> 
> 	* remote-sim.c (gdbsim_open): Add the strlen of " --sysroot=" and
> 	gdb_sysroot to the "len" variable.  Append both to "arg_buf".

It turns out that this is breaking some simulators, at least the ppc sim:

        % powerpc-elf-gdb
        (gdb) target sim
        Unrecognized option

The problem is that some sims have their own argument parsing,
and they sometimes reject the --sysroot= option.  Here is the patch
I just checked in to fix the ppc, but I think that other simulators
might have the same problem.  I checked a few, and I think that
the d10v sim, for instance, will also need to be updated).

So, we have a decision to make: Either do a pass over all simulators,
and fix the ones that need fixing, or revert (in which case my ppc
patch also needs to be reverted).

-- 
Joel

[-- Attachment #2: sim-ppc-sysroot-option.diff --]
[-- Type: text/x-diff, Size: 1488 bytes --]

commit df31576192c6a95c3a56583d7de54a452dd52110
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Wed Jun 8 09:17:09 2011 -0700

    ppc sim: Allow --sysroot command-line option
    
    There was a recent change that cuased the "target sim" command
    to add a --sysroot option to the argument vector passed down to
    the simulator.  This caused a failure in the powerpc simulator,
    as it did not recognize it.  This patch fixes the problem by adding
    support for the --sysroot option (it ignores it).
    
    sim/ppc/ChangeLog:
    
            * psim.c (psim_options): Accept and ignore `--sysroot=...'.

diff --git a/sim/ppc/ChangeLog b/sim/ppc/ChangeLog
index 2b9017a..2536fa4 100644
--- a/sim/ppc/ChangeLog
+++ b/sim/ppc/ChangeLog
@@ -1,3 +1,7 @@
+2011-06-08  Joel Brobecker  <brobecker@adacore.com>
+
+	* psim.c (psim_options): Accept and ignore `--sysroot=...'.
+
 2011-06-03  Joel Brobecker  <brobecker@adacore.com>  (obvious fix)
 
 	From Stephen Kitt  <steve@sk2.org>
diff --git a/sim/ppc/psim.c b/sim/ppc/psim.c
index c311794..076a50c 100644
--- a/sim/ppc/psim.c
+++ b/sim/ppc/psim.c
@@ -357,6 +357,10 @@ psim_options(device *root,
         }
 	else if (strcmp (argv[argp], "--help") == 0)
 	  psim_usage (0, 1);
+	else if (strncmp (argv[argp], "--sysroot=",
+			  sizeof ("--sysroot=")) == 0)
+	  /* Ignore this option.  */
+	  p = argv[argp] + strlen(argv[argp]) - 1;
 	else if (strcmp (argv[argp], "--version") == 0)
 	  {
 	    extern const char version[];

Re: [PATCH] gdb: sim: automatically pass down sysroot

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1133 bytes --]

On Wednesday, June 08, 2011 12:49:39 Joel Brobecker wrote:
> > 2011-05-26  Mike Frysinger  <vapier@gentoo.org>
> > 
> > 	* remote-sim.c (gdbsim_open): Add the strlen of " --sysroot=" and
> > 	gdb_sysroot to the "len" variable.  Append both to "arg_buf".
> 
> It turns out that this is breaking some simulators, at least the ppc sim:
> 
>         % powerpc-elf-gdb
>         (gdb) target sim
>         Unrecognized option
> 
> The problem is that some sims have their own argument parsing,
> and they sometimes reject the --sysroot= option.  Here is the patch
> I just checked in to fix the ppc, but I think that other simulators
> might have the same problem.  I checked a few, and I think that
> the d10v sim, for instance, will also need to be updated).
> 
> So, we have a decision to make: Either do a pass over all simulators,
> and fix the ones that need fixing, or revert (in which case my ppc
> patch also needs to be reverted).

i'll take a look over the sims to see which lack sysroot support and fix them 
up ... really wish people would use the common/ dir more as this keeps biting 
me :/
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] gdb: sim: automatically pass down sysroot

[Joel Brobecker]

> really wish people would use the common/ dir more as this keeps biting
> me :/

Same here.

-- 
Joel

Re: [PATCH] gdb: sim: automatically pass down sysroot

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 824 bytes --]

On Wednesday, June 08, 2011 12:49:39 Joel Brobecker wrote:
> > 2011-05-26  Mike Frysinger  <vapier@gentoo.org>
> > 
> > 	* remote-sim.c (gdbsim_open): Add the strlen of " --sysroot=" and
> > 	gdb_sysroot to the "len" variable.  Append both to "arg_buf".
> 
> It turns out that this is breaking some simulators, at least the ppc sim:

with your ppc fix and my erc32 patch, i dont see any other sims failing

these guys are using sim_args_command() and sim_parse_args() which handles the 
--sysroot so they should "just work" already:
	bfin cris frv iq2000 lm32 m32r m68hc11 mips mn10300 moxie sh64 v850

these guys appear to ignore all and/or unknown options:
	arm avr cr16 m32c mcore microblaze rx sh

these guys are no longer in gdb, so doesnt matter:
	d10v

so i think ive covered everyone ...
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] gdb: sim: automatically pass down sysroot

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 659 bytes --]

here's the fix for erc32.  ok to commit ?

(style is god awful, but seems the whole erc32 code base is buggered and i'm 
not about to try and fix that)
-mike

--- sim/erc32/interf.c	1 Jun 2011 17:35:02 -0000	1.10
+++ sim/erc32/interf.c	9 Jun 2011 05:03:32 -0000
@@ -234,6 +234,9 @@ sim_open (kind, callback, abfd, argv)
 		if ((stat + 1) < argc) {
 		    freq = strtol(argv[++stat], (char **)NULL, 0);
 		}
+	    } else
+	    if (strncmp(argv[stat], "--sysroot=", sizeof("--sysroot=")) == 0) {
+		/* Ignore until we start to support this.  */
 	    } else {
 		(*sim_callback->printf_filtered) (sim_callback,
 						  "unknown option %s\n",

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] gdb: sim: automatically pass down sysroot

[Joel Brobecker]

> here's the fix for erc32.  ok to commit ?

Yes, please. Thank you for taking care of that. Funny I didn't notice,
this problem, but it turns out that the error is non-fatal...

And good to know that this is the last simulator that should be
impacted.  If there is a relatively mechanichal way to use sim/common/,
we can look at that option, since the number of simulators to update
would be small (2?).

> (style is god awful, but seems the whole erc32 code base is buggered and i'm 
> not about to try and fix that)

I had the exact same reaction - it's just amazing how style differences
can make the reading so much more difficult.

> --- sim/erc32/interf.c	1 Jun 2011 17:35:02 -0000	1.10
> +++ sim/erc32/interf.c	9 Jun 2011 05:03:32 -0000
> @@ -234,6 +234,9 @@ sim_open (kind, callback, abfd, argv)
>  		if ((stat + 1) < argc) {
>  		    freq = strtol(argv[++stat], (char **)NULL, 0);
>  		}
> +	    } else
> +	    if (strncmp(argv[stat], "--sysroot=", sizeof("--sysroot=")) == 0) {
> +		/* Ignore until we start to support this.  */
>  	    } else {
>  		(*sim_callback->printf_filtered) (sim_callback,
>  						  "unknown option %s\n",

-- 
Joel

Re: [PATCH] gdb: sim: automatically pass down sysroot

[Mike Frysinger]

On Thu, Jun 9, 2011 at 01:47, Joel Brobecker wrote:
> And good to know that this is the last simulator that should be
> impacted.  If there is a relatively mechanichal way to use sim/common/,
> we can look at that option, since the number of simulators to update
> would be small (2?).

"no" to all parts unfortunately.  the sim/common/ tree is a loose
collection of files.  sims get to opt in to different pieces or simply
re-implement the core functions themselves.  i'd say probably more
than half do.  you can tell quickly by looking at the sub-Makefile and
see how many objects from common it refers to.  and if the sim still
uses "run.c" instead of "nrun.c", you're really screwed.

and from what i recall from when i converted bfin from standalone to
the common tree, the main loops, header files, and state are heavily
impacted.  fairly invasive to convert over, but much of it is simply
code massage rather than rewriting new stuff.

the upside is that the conversion tends to involve throwing out a lot
of code because common/ takes care of it for you.  the downside is
that it isnt trivial, and i unfortunately have no interest in
converting other sims.
-mike

Re: [PATCH] gdb: sim: automatically pass down sysroot

[Tristan Gingold]

On Jun 9, 2011, at 7:07 AM, Mike Frysinger wrote:

> here's the fix for erc32.  ok to commit ?

Is it correct ?  IIRC sizeof ("xxx") == strlen ("xxx") + 1.

> (style is god awful, but seems the whole erc32 code base is buggered and i'm 
> not about to try and fix that)
> -mike
> 
> --- sim/erc32/interf.c	1 Jun 2011 17:35:02 -0000	1.10
> +++ sim/erc32/interf.c	9 Jun 2011 05:03:32 -0000
> @@ -234,6 +234,9 @@ sim_open (kind, callback, abfd, argv)
> 		if ((stat + 1) < argc) {
> 		    freq = strtol(argv[++stat], (char **)NULL, 0);
> 		}
> +	    } else
> +	    if (strncmp(argv[stat], "--sysroot=", sizeof("--sysroot=")) == 0) {
> +		/* Ignore until we start to support this.  */
> 	    } else {
> 		(*sim_callback->printf_filtered) (sim_callback,
> 						  "unknown option %s\n",

Re: [PATCH] gdb: sim: automatically pass down sysroot

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 349 bytes --]

> > here's the fix for erc32.  ok to commit ?
> 
> Is it correct ?  IIRC sizeof ("xxx") == strlen ("xxx") + 1.

You're quite right.  The weird thing is that I had the "- 1"
in my code at some point, I'm sure of that. How it came to
disappear, I'm really puzzled.

Here is what I just checked in to fix the problem on ppc.
Thanks, Tristan.

-- 
Joel

[-- Attachment #2: sim-ppc.diff --]
[-- Type: text/x-diff, Size: 1435 bytes --]

commit 1f192d6b369336703e07d74833f98a8bdc6c3361
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Thu Jun 9 07:18:36 2011 -0700

    sim/ppc: Fix check for --sysroot= option
    
    Fixes an error reported by Tristan and which can be evidenced by
    doing:
    
            % powerpc-elf-gdb
            (gdb) target sim --sysroot=var
            Invalid option: --sysroot=/var
            [...]
    
    sim/ppc/ChangeLog:
    
            * psim.c (psim_options): Fix length of comparison when checking
            for --sysroot= option.

diff --git a/sim/ppc/ChangeLog b/sim/ppc/ChangeLog
index 5e6fff6..455585a 100644
--- a/sim/ppc/ChangeLog
+++ b/sim/ppc/ChangeLog
@@ -1,3 +1,8 @@
+2011-06-09  Joel Brobecker  <brobecker@adacore.com>
+
+	* psim.c (psim_options): Fix length of comparison when checking
+	for --sysroot= option.
+
 2011-06-08  Joel Brobecker  <brobecker@adacore.com>
 
 	* psim.c (psim_options): Add option that cause the error
diff --git a/sim/ppc/psim.c b/sim/ppc/psim.c
index d814486..3e76386 100644
--- a/sim/ppc/psim.c
+++ b/sim/ppc/psim.c
@@ -359,7 +359,7 @@ psim_options(device *root,
 	else if (strcmp (argv[argp], "--help") == 0)
 	  psim_usage (0, 1);
 	else if (strncmp (argv[argp], "--sysroot=",
-			  sizeof ("--sysroot=")) == 0)
+			  sizeof ("--sysroot=") - 1) == 0)
 	  /* Ignore this option.  */
 	  p = argv[argp] + strlen(argv[argp]) - 1;
 	else if (strcmp (argv[argp], "--version") == 0)

Re: [PATCH] gdb: sim: automatically pass down sysroot

[Joel Brobecker]

> the upside is that the conversion tends to involve throwing out a lot
> of code because common/ takes care of it for you.  the downside is
> that it isnt trivial, and i unfortunately have no interest in
> converting other sims.

Thanks. I would probably have taken care of erc32 and ppc, as I have
some ways of testing them (I have the compilers). But if it isn't
mechanical, I have more pressing work to do as well...

-- 
Joel

Re: [PATCH] gdb: sim: automatically pass down sysroot

[Mike Frysinger]

On Thu, Jun 9, 2011 at 10:24, Joel Brobecker wrote:
>> > here's the fix for erc32.  ok to commit ?
>>
>> Is it correct ?  IIRC sizeof ("xxx") == strlen ("xxx") + 1.
>
> You're quite right.  The weird thing is that I had the "- 1"
> in my code at some point, I'm sure of that. How it came to
> disappear, I'm really puzzled.

i was working with constants originally, but when i looked at Joel's
patch, i copied & pasted what he did.  so the erc32 version i
committed includes his latest change.
-mike

--- interf.c    1 Jun 2011 17:35:02 -0000       1.10
+++ interf.c    9 Jun 2011 15:30:54 -0000
@@ -234,6 +234,9 @@ sim_open (kind, callback, abfd, argv)
                if ((stat + 1) < argc) {
                    freq = strtol(argv[++stat], (char **)NULL, 0);
                }
+           } else
+           if (strncmp(argv[stat], "--sysroot=", sizeof("--sysroot=")
- 1) == 0) {
+               /* Ignore until we start to support this.  */
            } else {
                (*sim_callback->printf_filtered) (sim_callback,
                                                  "unknown option %s\n",

Re: [PATCH] gdb: sim: automatically pass down sysroot

[Mike Frysinger]

On Fri, May 27, 2011 at 13:46, Tom Tromey wrote:
> Mike> +  strcat (arg_buf, gdb_sysroot);
>
> It seems like this will give wrong results if the sysroot needs quoting.
> But, this problem could affect other arguments here.
> Offhand it seems like it would be better if this code directly built
> sim_argv rather than building a string and converting it, but I don't
> know if any odd issue is lurking.

i thought of that, but since as you say other parts have the same
issue, and i couldnt find a helper func that would sanely take care of
things, i didnt bother
-mike

Re: [PATCH] gdb: sim: automatically pass down sysroot

[Tom Tromey]

>>>>> "Mike" == Mike Frysinger <vapier@gentoo.org> writes:

Mike> 2011-05-26  Mike Frysinger  <vapier@gentoo.org>
Mike> 	* remote-sim.c (gdbsim_open): Add the strlen of " --sysroot=" and
Mike> 	gdb_sysroot to the "len" variable.  Append both to "arg_buf".

It seems reasonable to me.  Ok.

Mike> +  strcat (arg_buf, gdb_sysroot);

It seems like this will give wrong results if the sysroot needs quoting.
But, this problem could affect other arguments here.
Offhand it seems like it would be better if this code directly built
sim_argv rather than building a string and converting it, but I don't
know if any odd issue is lurking.

Tom