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

[Luis Machado <lgustavo@codesourcery.com>]

On 06/19/2013 11:10 AM, Pedro Alves wrote:
> 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.  ;-)
>

Ok. That's good. :-)

I'll wait for Mike's feedback before attempting any other changes for 
this particular issue.

Luis