On 06/01/2022 09:53, Dmitry Yemanov wrote:
> 06.01.2022 04:28, Adriano dos Santos Fernandes wrote:
>>
>> There is ExprNode::FLAG_VALUE ("Full value area required in impure
>> space"), inherited from old (2.5) code base nod_value.
>>
>> It's set by sort subsystem and used only for parameters and variables.
>
> Initially it as also used for fields. But at some point of time fields's
> impure area became always sizeof(impure_value_ex), unconditionally.
>
>> It makes then allocate impure space for impure_value_ex instead of
>> traditional dsc.
>
> I'd say allocating sizeof(dsc) is dangerous for these nodes. Look at
> EVL_assign_to(), for example. It does:
>
> impure_value* impure =
> (impure_value*) ((SCHAR *) request + node->nod_impure);
> ...
> impure->vlu_desc.dsc_dtype = desc->dsc_dtype;
> ...
> return &impure->vlu_desc;
>
> i.e. it works just because impure_value has "dsc vlu_desc" at the first
> position and thus the used part of impure_value is equal to sizeof(dsc).
> Change that and the things get broken.
I'd say this code (looking at master) can be improved.
It's very weird for parameters. EVL_assign_to gets the descriptor inside
the ParameterNode. But multiple parameters usage in code results in
multiple ParameterNodes.
Instead of returning a dsc*, it seems better if EVL_assign_to accepts a
pointer/reference to a dsc, "allocated" as local variable in its callers.
This may works also for fields.
For variables, it already reassigns the impure variable and gets the
descriptor from the DeclareVariableNode.
> Try to use vlu_flags for
> arguments/variables (make them invariant, for example) and things get
> broken. Too fragile, IMO. I'd consider allocating sizeof(impure_value)
> instead of sizeof(dsc), just for safety sake.
>
FLAG_INVARIANT is marked by nodes for they own, so they must be
compatible with impure_value.
vlu_flags is used in the DeclareVariableNode for variables, so it looks ok.
Weird things happens in UdfCallNode.
And it's why it have this code (comment):
struct Impure
{
impure_value value; // must be first
Firebird::Array<UCHAR>* temp;
};
>> Most nodes allocate space for impure_value. But not all of them.
>>
>> Literals directly return the descriptor set in compile time.
>>
>> I see no usage of the expressions impure_value in sort. And if they were
>> using, we'd certainly have a problem with literals.
>
> See also:
>
> https://github.com/FirebirdSQL/firebird/issues/1355
>
> https://github.com/FirebirdSQL/firebird/commit/626ab18c426fd32d482e02093e72e57330596174
>
>
> Worth testing GROUP BY <literal>, <field> ?
>
Since v3 we must be safe. Aggregate stream allocates impure space for
the expressions in its own region.
I'd say the flag exists because things were different before. Aggregate
were using expressions' impure regions.
Adriano
Firebird-Devel mailing list, web interface at
https://lists.sourceforge.net/lists/listinfo/firebird-devel
