[RFA_v4,1/8] Add helper functions parse_flags and parse_flags_qcs

  On Tue, 2018-07-31 at 09:40 -0600, Tom Tromey wrote:
> > > > > > "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
> > > I also found this bug this weekend, while trying out -fsanitize=address.
> > > 
> > > I have a patch for this one.  I haven't written the ChangeLog yet but I
> > > will try to do it as soon as possible.
> > > 
> > > Actually I have patches to make gdb nearly -fsanitize=address clean; or
> > > at least, the ordinary test suite on my machine is down to 1 failure
> > > (there are some additional gdbserver failures in bugzilla that I haven't
> > > looked at yet).  My series also addresses much of -fsanitize=undefined
> > > as well.
> 
> Joel> Very nice!
> 
> You may have to wait a bit longer because the buildbot is telling me
> this patch is no good.  I'll try to debug it tonight.

As far as I can see, this problem is a regression that appeared in gdb 8.1,
but which was made (more) visible by the 'parse_flags' patch in (future) 8.3.
At least valgrind + gdb 8.0 does not give a problem with the small reproducer 
  command 1 2
  end
while it gives an error with gdb 8.1.

We also have a (small) functional regression:
with gdb 8.0, it was possible to remove all commands of a set of
breapoints by doing the above 'command 1 2/end'.

From 8.1 onwards, when giving such empty command list, gdb asks
the list of command for each breakpoint, instead of asking it once.

The below patch seems to solve the memory corruption (at least
for the simple case), but does not solve the functional regression that
appeared in 8.1.

I am wondering if the correct solution would not be to avoid
having input lines memory being managed 'manually' like it is now,
as having the 'input const char* arg' disappearing 'under the carpet'
is quite tricky, and we might have other places where a previous
line of input must be kept alive, while new lines of input have
to be read.

Philippe
  

>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

Philippe> I am wondering if the correct solution would not be to avoid
Philippe> having input lines memory being managed 'manually' like it is now,
Philippe> as having the 'input const char* arg' disappearing 'under the carpet'
Philippe> is quite tricky, and we might have other places where a previous
Philippe> line of input must be kept alive, while new lines of input have
Philippe> to be read.

Yeah, my approach has been to require the callers of
handle_line_of_input to handle the storage, so nothing relies on
saved_command_line surviving across reentrant calls.

Thanks for pointing out that other regression.  I don't think I was
aware of that one.

Tom
  

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
Philippe> I am wondering if the correct solution would not be to avoid
Philippe> having input lines memory being managed 'manually' like it is now,
Philippe> as having the 'input const char* arg' disappearing 'under the carpet'
Philippe> is quite tricky, and we might have other places where a previous
Philippe> line of input must be kept alive, while new lines of input have
Philippe> to be read.

Tom> Yeah, my approach has been to require the callers of
Tom> handle_line_of_input to handle the storage, so nothing relies on
Tom> saved_command_line surviving across reentrant calls.

The bug in my patch turns out to be this line in top.c:

      if (repeat_arguments != NULL && cmd_start == saved_command_line)

... which relies on handle_line_of_input returning saved_command_line
exactly rather than a copy.

Fixing this in a good way will require passing a bit more information
through the maze of functions.  This part of gdb sure seems complicated
for what it does.

A less principled way to fix this would be to stuff another global in
there somewhere, to indicate that the current command is a repeat.  This
is "safe" in that nearly nothing ought to check the global.  However, I
think I'd rather try not to introduce more globals, at least unless I
get too annoyed.

Tom