Re: [PATCH 000/203] Refactor expressions

Joel Brobecker
2021-01-03T07:03:03Z

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.

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