Re: [PATCH, gdbsim] Avoid silly crash when no binary is loaded

[Luis Machado <lgustavo at codesourcery dot com>]

Hi,

On 06/19/2013 08:19 AM, Pedro Alves wrote:
> On 06/18/2013 09:49 PM, Luis Machado wrote:
> > Hi,
> >
> > This patch prevents the long-standing crash scenario where we start
> > gdbsim and "run" without any binaries. Warnings are issued, but those
> > don't prevent the simulator from proceeding with garbage data.
>
> Which sim and backtrace?  I suspect this to be sim/arch dependent.

This is arm. Other simulators (mips and powerpc) have different 
behaviors. No crashes, but they go all over the place in terms of messages.

I'm questioning the use case of attempting to let the simulator go 
without loading any image to it. If it is useful, then we should state 
that and make it stop crashing.

There is already a barrier, see 
remote-sim.c:gdbsim_xfer_inferior_memory. The same message will be 
displayed there with an error.

  if (!sim_data->program_loaded)
    error (_("No program loaded."));

So, in a way, we're already preventing this scenario later on. If we 
want to keep the old behavior, for whatever old reason that may be, i'm 
ok with it.

#0  0x00000000006a0580 in ARMul_SetPC (state=0x0, value=0) at 
../../../gdb-head/sim/arm/armsupp.c:83
#1  0x0000000000690cef in sim_create_inferior (sd=0x1, abfd=0x0, 
argv=0x0, env=0xc21d90) at ../../../gdb-head/sim/arm/wrapper.c:249
#2  0x0000000000456a93 in gdbsim_create_inferior (target=0xb58100 
<gdbsim_ops>, exec_file=0x0, args=0xc39df0 "", env=0xc21d90, from_tty=1) 
at ../../gdb-head/gdb/remote-sim.c:646
#3  0x0000000000575b09 in target_create_inferior (exec_file=0x0, 
args=0xc39df0 "", env=0xc21d90, from_tty=1) at 
../../gdb-head/gdb/target.c:508
#4  0x000000000052a78a in run_command_1 (args=0x0, from_tty=1, 
tbreak_at_main=0) at ../../gdb-head/gdb/infcmd.c:585
#5  0x000000000052a885 in run_command (args=0x0, from_tty=1) at 
../../gdb-head/gdb/infcmd.c:617
#6  0x0000000000459993 in do_cfunc (c=0xbe3f80, args=0x0, from_tty=1) at 
../../gdb-head/gdb/cli/cli-decode.c:113
#7  0x000000000045cab7 in cmd_func (cmd=0xbe3f80, args=0x0, from_tty=1) 
at ../../gdb-head/gdb/cli/cli-decode.c:1888
#8  0x000000000064e735 in execute_command (p=0xb606c3 "", from_tty=1) at 
../../gdb-head/gdb/top.c:489
#9  0x0000000000555327 in command_handler (command=0xb606c0 "") at 
../../gdb-head/gdb/event-top.c:433
#10 0x000000000055590a in command_line_handler (rl=0xc2d7c0 "run") at 
../../gdb-head/gdb/event-top.c:631
#11 0x00000000006e60da in rl_callback_read_char () at 
../../gdb-head/readline/callback.c:220
#12 0x0000000000554e51 in rl_callback_read_char_wrapper 
(client_data=0x0) at ../../gdb-head/gdb/event-top.c:164
#13 0x000000000055523e in stdin_event_handler (error=0, client_data=0x0) 
at ../../gdb-head/gdb/event-top.c:373
#14 0x0000000000553dd6 in handle_file_event (data=...) at 
../../gdb-head/gdb/event-loop.c:768
#15 0x000000000055327f in process_event () at 
../../gdb-head/gdb/event-loop.c:342
#16 0x0000000000553346 in gdb_do_one_event () at 
../../gdb-head/gdb/event-loop.c:406
#17 0x0000000000553397 in start_event_loop () at 
../../gdb-head/gdb/event-loop.c:431
#18 0x0000000000554e7b in cli_command_loop () at 
../../gdb-head/gdb/event-top.c:177
#19 0x000000000054b7db in current_interp_command_loop () at 
../../gdb-head/gdb/interps.c:331
#20 0x000000000054c280 in captured_command_loop (data=0x0) at 
../../gdb-head/gdb/main.c:260
#21 0x0000000000549c1a in catch_errors (func=0x54c265 
<captured_command_loop>, func_args=0x0, errstring=0x810024 "", mask=6) 
at ../../gdb-head/gdb/exceptions.c:546
#22 0x000000000054d6c4 in captured_main (data=0x7fffffffe080) at 
../../gdb-head/gdb/main.c:1055
#23 0x0000000000549c1a in catch_errors (func=0x54c51c <captured_main>, 
func_args=0x7fffffffe080, errstring=0x810024 "", mask=6) at 
../../gdb-head/gdb/exceptions.c:546
#24 0x000000000054d6fa in gdb_main (args=0x7fffffffe080) at 
../../gdb-head/gdb/main.c:1064
#25 0x000000000040602a in main (argc=1, argv=0x7fffffffe188) at 
../../gdb-head/gdb/gdb.c:34

> > Replacing those warnings with error calls seems to be the most
> > appropriate here.
>
> Well, the code seems to have been written like that for a reason.
>
> Real boards can be powered on with no real program in memory
> too...

Of course. The question is if there is any useful use case of letting 
the simulator run without any images loaded.

> >     if (exec_file == 0 || exec_bfd == 0)
> > -    warning (_("No executable file specified."));
> > +    error (_("No executable file specified."));
> >     if (!sim_data->program_loaded)
> > -    warning (_("No program loaded."));
> > +    error (_("No program loaded."));
>
> There's code just below that does:
>
> >     if (remote_debug)
> >       printf_filtered ("gdbsim_create_inferior: exec_file \"%s\", args \"%s\"\n",
> ...
> >   if (exec_file != NULL)
> >     {
> >       len = strlen (exec_file) + 1 + strlen (args) + 1 + /*slop */ 10;
> >       arg_buf = (char *) alloca (len);
> >       arg_buf[0] = '\0';
> >       strcat (arg_buf, exec_file);
> >       strcat (arg_buf, " ");
> >       strcat (arg_buf, args);
> >       argv = gdb_buildargv (arg_buf);
> >       make_cleanup_freeargv (argv);
> >     }
> >   else
> >     argv = NULL;
>
> So if we error out, then these NULL checks are now dead.

Right. This may turn to be dead code and may need removal.

> And e.g., the bfin sim, at sim/bfin/interp.c:sim_create_inferior
> allows NULL exec_bfd:
>
> SIM_RC
> sim_create_inferior (SIM_DESC sd, struct bfd *abfd,
> 		     char **argv, char **env)
> {
>    SIM_CPU *cpu = STATE_CPU (sd, 0);
>    SIM_ADDR addr;
>
>    /* Set the PC.  */
>    if (abfd != NULL)
>      addr = bfd_get_start_address (abfd);
>    else
>      addr = 0;
>    sim_pc_set (cpu, addr);

Is there a good reason why bfin would allow things to proceed without 
any image? It doesn't even run past that point really.

All i see, for whatever operation, is "No memory".

ppc gives me "No program loaded", "The program is not being run" and 
"The program has no registers now"

mips says "sim_monitor: unhandled reason = 0, pc = 0xbfc00000", then 
falls into the old "Cannot execute this command while the selected 
thread is running" or "sim-events.c:231: assertion failed - 
events->resume_wallclock == 0".

If running, and by that i mean issuing run/start/continue/step commands, 
the simulators with no image is a valid use case, then sounds like 
steering the arm simulator to just do more or less what the other 
simulators do is the right thing.

If the use case is not useful at all, i think we should just wipe it out 
rather than preserve some old unclear feature.

Then again, these simulators are old and not used that often.

Luis