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

[Luis Machado <lgustavo@codesourcery.com>]

Thought it would be best to cc Mike.

On 06/19/2013 12:15 PM, Luis Machado wrote:
> 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
>
>