Re: [PATCH 000/203] Refactor expressions

Andrew Burgess
2021-01-04T12:16:38Z

* Joel Brobecker <brobecker@adacore.com> [2021-01-03 11:02:50 +0400]:

> Hi Tom,
>
> On Fri, Jan 01, 2021 at 02:44:00PM -0700, Tom Tromey wrote:
> > I have long thought that struct expression is the worst data structure
> > in GDB. In fact, I've sometimes told people it is the worst data
> > structure I've encountered in my career. I've wanted to rewrite it
> > for many years, and this year I finally found a workable plan and the
> > motivation to do so. This series is the result.
>
> Thanks for the (mega) series!
>
> Because of the length of the series, I think it's going to be potentially
> more difficult for you to maintain it over time. I so I think it would
> be nice if we could put a priority on its review.
>
> I've now started with part 1:
>
> > 1. Split functions out from the evaluators.
> >
> > Each evaluator function is fairly long, and mixes knowledge about
> > the structure of the expression (how the exp_elements are laid out)
> > with knowledge of the semantics of the operation.
> >
> > This part of the series, which is about 70 patches, splits out the
> > semantics of most operations into their own functions. These
> > functions can be then be reused -- which is helpful for ensuring
> > that changes don't sneak in.
> >
> > In this part of the series, sometimes the newly-introduced
> > functions have extraneous arguments. This was done partly so that
> > some could be reused by generic code in part 2; but also partly
> > because it was just simpler to write patches by rote.
>
> Aside from the commit "what is this code for", I think this part
> of the series is a nice improvement on its own. independently
> of redesigning struct expression, I've always found the evaluator
> functions to be overly long and harder to navigate as a result.
> So I think it could go in ahead of the rest if we agree that
> this part is good.
>
> For me, I've gone through the patches, more or less carefully
> based on a random sample, and they look good. I paused a bit
> about the Ada ones, were you excluded the hanlding of noside ==
> EVAL_SKIP. I'm not entirely sure why that is, perhaps because
> the block consists in a goto nosideret? Looking at what that
> nosideret does, it's just...
>
> | nosideret:
> | return eval_skip_value (exp);
>
> ... so we could inline this in the new functions.
>
> However, this is really a very minor detail that doesn't need to be
> addressed here.
>
> So, to summarize, if others agree, I'm happy for this part of
> the series to go in.

I agree with this. I looked through all the patches 001 -> 072.
There's a few missing comments that I think should be added, and I
would suggest dropping the "what is this code for" patch (if this
needs looking at it can be a separate branch), but otherwise this
first set would be a good improvement on its own and worth merging
sooner rather than later I think.

Thanks,
Andrew

>
> >
> > 2. Introduce 'class operation' and its subclasses.
> >
> > This sub-series is around 100 patches. It introduces the new base
> > class for an expression operation, and then proceeds to add
> > operations for every necessary opcode. In some cases multiple such
> > operations are needed -- for example when multiple languages
> > implement an opcode in different ways.
> >
> > Some discussion of the design & tradeoffs appear in the "Introduce
> > class operation" patch.
> >
> > 3. Convert existing code to use operations.
> >
> > This is a short sub-series of around 10 patches. Each parser is
> > converted separately, as are DTrace and SystemTap probes
> >
> > 4. Delete obsolete code.
> >
> > The final 20 patches or so are concerned with removing all the code
> > that is now obsolete, and with doing some minor tidying of the
> > result.
> >
> > The overall approach here is to replace the current data structure --
> > but only the data structure. I didn't generally try to clean up
> > things that seemed a bit weird.
> >
> > I also didn't try to add new features, like async evaluation. After
> > starting the project, I felt that trying to combine this refactoring
> > with another, different one would be too difficult.
> >
> > This is all on the branch t/expression-rewrite-incr in my github.
> >
> > Regression tested on x86-64 Fedora 32. I also built it on PPC Linux
> > to ensure that the ppc-linux-nat.c changes would build.
> >
> > Tom
> >
>
> --
> Joel