Re: [PATCH 01/43] Replace the symbol needs evaluator with a parser

Zoran Zaric via Gdb-patches
2021-04-28T15:02:53Z

>>>> Some concerns were raised that a linear scan of the expression byte
>>>> stream would have issues if a DWARF producer would try to hide some
>>>> non DWARF related data after a control flow operation. Although the
>>>> testsuite doesn't show this case, it is a valid concern, so one of
>>>> the later patches in this series will address it by switching back to
>>>> the then redesigned DWARF expression evaluator.
>>>
>>> While re-reading your exchange with Tom, I was under the impression that
>>> traversing the "control flow graph" of the expression, visiting each
>>> node only once, would be a good solution. It would avoid the infinite
>>> loop problem, the "two branches" problem, and even the corner cases
>>> where you have garbage in the middle of the expression, or if the
>>> expression jumps in the middle of an instruction to re-use the operand
>>> of an instruction as an instruction.
>>>
>>> Patch 39 changes this back to an evaluator, so I'm not sure if this is
>>> what you implemented or something else, I'll see when I get there.
>>
>> There is no concept of a "control flow graph" in the existing gdb evaluator, there is just a byte stream that gdb either fully traverse and evaluates or it "fakes" the target access.
>
> When I meant "control flow graph", I didn't mean to have a proper
> control flow graph representation with classes and all (unless we think
> it would be useful), but just navigate it conceptually.
>
> You maintain a queue of "to visit" instructions and a set of "already
> visited" instructions. You by pushing the first instruction in the
> queue and work until the queue is empty. For all instructions but
> conditional jumps, you push the instruction after it in the queue. For
> conditional jumps, you also push the jump target in the queue. You
> maintain the "already visited" set on the side to avoid visiting the
> same instruction twice, in case there are loops.

Right, this would probably work, but the only problem it is solving is
the garbage issue, and considering that the later patch in the series
removes this temporary solution anyway. I don't think it is worth the
time in re-implementing it.

>
>>
>> The solution that Tom talked about was just an idea that he used in some other tool and would need to be implemented from scratch.
>>
>> The patch 39 switches the evaluator design to throwing the missing context exception idea which re-enables using the same evaluator for both purposes.
>
> I'll see and understand better when I get there. But from what I
> understood of past dicussions, using the evaluator may give wrong
> results, because one branch may require only registers while another
> branch may require a frame (that's the point of your patch). So I don't
> really understand how going back to using an evaluator would work.

You can't really have a register without a frame. That doesn't really
make sense in any of the existing targets. Even the struct value
describing the register location always needs to have a frame defined.

The question that symbol needs is answering is if the symbol needs a
frame or not. And this is why catching an exception thrown by the
evaluator when the frame was needed but missing does work.

>
> Although that depends on the semantic we want symbol_needs to have:
>
> - find what's needed to evaluate the expression in the current context,
> only in the branch that would be taken right now?
> - find what's needed to evaluate the expression in any possible
> context, considering all branches?
>
> Simon
>

Symbol needs is always called regarding the current context, I didn't
see anything that proves otherwise.

Zoran