RE: [PATCH v2] Replace the symbol needs evaluator with a parser

["Zaric, Zoran (Zare)" <Zoran.Zaric@amd.com>]

[AMD Official Use Only - Internal Distribution Only]

> >>>>> "Zoran" == Zoran Zaric <Zoran.Zaric@amd.com> writes:
> 
> Zoran> The problem here is that faking results of target interactions
> Zoran> can yield an incorrect evaluation result.
> 
> ...
> 
> Zoran> This is clearly a wrong result and it causes the debugger to crash.
> 
> Zoran> +      /* The DWARF expression might have a bug causing an infinite
> Zoran> +         loop.  In that case, quitting is the only way out.  */
> Zoran> +      QUIT;
> 
> Can this really occur in this scanner?
> 
> For evaluation I can understand it, but this scanner seems to just walk the
> bytecode once, so I don't see how it is possible.

It is not. 

The test is exposing a bug in the previous evaluator based approach. My point was to prevent people going back to that approach.

> 
> For gimli (again) we have a maximum operation count to avoid this kind of
> problem.  gdb could do this as well.
> 
> Meanwhile, about the linear scan -- it seems to me that nothing in DWARF
> requires (1) that the expression not contain embedded garbage, and (2) that
> it not be possible to branch to the middle of some other instruction.  (If
> there is text along these lines, I'd like to hear about it; I looked in the past
> and couldn't find it.)

I am not sure about your point about  expressions not contain embedded garbage. The DWARF expression is a byte stream with clearly defined content for every operation.

Regarding jumping in the middle of another operation, this is true, DWARF doesn't prevent that or define what is supposed to be done.

And I agree it would be really useful to have a checking mechanism that converts that byte stream into an internal representation, where during that conversion we could check and report all kind of things that can be wrong with the expression.

> This is pathological, of course.  But on the other hand, if the motivation is to
> avoid crashes, it seems bad to, say, let this function pass through some
> bytecode that would then be interpreted a different way by the actual
> evaluator.
> 
> Personally I'd be fine with rejecting this bad stuff from the evaluator as well.
> But anyway the difficulty is that they have to be in sync, and aren't.
> 
> Another approach would be to record which offsets are branched to; and to
> pop from such a list when an unconditional branch is encountered.
> This would solve both problems as well.

I agree with all your points and to be honest I am not advocating for symbol needs mechanism (new or old) at all, and nothing would make me happier then to get rid of it.

My next patch series is supposed to redesign the DWARF evaluator mechanism in a way that it can be used in places where the symbol needs is currently used. The downside of that approach is that the scanner is still going to be considerable faster, but maybe we don't really care about that.

If at that time we feel that it is better to use the new evaluator instead of the new symbol need scanner, I would be happy to do the switch.

Zoran