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

[Pedro Alves <palves@redhat.com>]

On 06/19/2013 01:11 PM, Luis Machado wrote:
> 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.

I don't really know.  All I see is that from the code at it was
supported at least at some point.

> 
> 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

Curious.  'state' is initialized by the ARM sim's 'init' function in
the same file, and init is called only by sim_write, sim_read,
sim_store_register and sim_fetch_register.  'init' ends up
getting called by "load", through sim_load ->  sim_load_file -> sim_write.

> #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


>>>
>>> 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.

I'll leave that up to Mike.

> 
>>>     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.

I have no doubt it ends up as dead code.  ;-)  The patch just
looks obviously incomplete as is, and that prompted my reply.

> 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".

Leaving to Mike.  I just picked bfin because it's a maintained sim.

> 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.

Thank you -- all this analysis is much clearer and a stronger
rationale than the original "silly", or just calling out
that things seem appropriate with no backing.  ;-)

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

-- 
Pedro Alves