Re: [Xen-devel] [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Markus Armbruster <armbru@xxxxxxxxxx>]

Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> writes:

> 16.03.2020 11:21, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> writes:
>>
>>> On 14.03.2020 00:54, Markus Armbruster wrote:
>>>> Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> writes:
>>>>
>>>>> 13.03.2020 18:42, Markus Armbruster wrote:
>>>>>> Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> writes:
>>>>>>
>>>>>>> 12.03.2020 19:36, Markus Armbruster wrote:
>>>>>>>> I may have a second look tomorrow with fresher eyes, but let's get this
>>>>>>>> out now as is.
>>>>>>>>
>>>>>>>> Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> writes:
[...]
>>>>>>>>> +@@
>>>>>>>>> +
>>>>>>>>> + fn(..., Error ** ____, ...)
>>>>>>>>> + {
>>>>>>>>> +     ...
>>>>>>>>> +     Error *local_err = NULL;
>>>>>>>>> +     ... when any
>>>>>>>>> +     Error *local_err2 = NULL;
>>>>>>>>> +     ... when any
>>>>>>>>> + }
>>>>
>>>> This flags functions that have more than one declaration along any
>>>> control flow path.  It doesn't flag this one:
>>>>
>>>>       void gnat(bool b, Error **errp)
>>>>       {
>>>>           if (b) {
>>>>               Error *local_err = NULL;
>>>>               foo(arg, &local_err);
>>>>               error_propagate(errp, local_err);
>>>>           } else {
>>>>               Error *local_err = NULL;
>>>>               bar(arg, &local_err);
>>>>               error_propagate(errp, local_err);
>>>>           }
>>>>       }
>>>>
>>>> The Coccinelle script does the right thing for this one regardless.
>>>>
>>>> I'd prefer to have such functions flagged, too.  But spending time on
>>>> convincing Coccinelle to do it for me is not worthwhile; I can simply
>>>> search the diff produced by Coccinelle for deletions of declarations
>>>> that are not indented exactly four spaces.
>>>>
>>>> But if we keep this rule, we should adjust its comment
>>>>
>>>>       // Warn several Error * definitions.
>>>>
>>>> because it sure suggests it also catches functions like the one I gave
>>>> above.
>>>
>>> Hmm, yes.. We can write "Warn several Error * definitions in _one_
>>> control flow (it's not so trivial to match _any_ case with several
>>> definitions with coccinelle)" or something like this.
>>
>> Ha, "trivial" reminds me of a story.  The math professor, after having
>> spent a good chunk of his lecture developing a proof on the blackboad
>> turns to the audience to explain why this little part doesn't require
>> proof with the words familiar to any math student "and this is trivial."
>> Pause, puzzled look...  "Is it trivial?"  Pause, storms out of the
>> lecture hall.  A minute or three pass.  Professor comes back beaming,
>> "it is trivial!", and proceeds with the proof.
>>
>> My point is: it might be trivial with Coccinelle once you know how to do
>> it.  We don't.
>>
>> Suggest "(can't figure out how to match several definitions regardless
>> of control flow)".
>
> Wrong too, because I can:) for example, chaining two rules, catching the
> positions of definition and check that they are different.. Or, some
> cheating with python script.. That's why I wrote "not trivial",
>
> So, most correct would be "(can't figure out how to simply match several 
> definitions regardless
>> of control flow)".

Works for me.

> But again, coccinelle is for matching control flows, so its probably 
> impossible to match such thing..
[...]
>>> OK, I almost OK with it, the only thing I doubt a bit is the following:
>>>
>>> We want to keep rule1.local_err inheritance to keep connection with
>>> local_err definition.
>>
>> Yes.
>>
>>> Interesting, when we have both rule1.fn and rule1.local_err inherited,
>>> do we inherit them in separate (i.e. all possible combinations of fn
>>> and local_err symbols from rule1) or do we inherit a pair, i.e. only
>>> fn/local_err pairs, found by rule1? If the latter is correct, that
>>> with your script we loss this pair inheritance, and go to all possible
>>> combinations of fn and local_err from rule1, possibly adding some wrong
>>> conversion (OK, you've checked that no such cases in current code tree).
>>
>> The chaining "identifier rule1.FOO" is by name.  It's reliable only as
>> long as there is exactly one instance of the name.
>>
>> We already discussed the case of the function name: if there are two
>> instances of foo(), and rule1 matches only one of them, then we
>> nevertheless apply the rules chained to rule1 to both.  Because that can
>> be wrong, you came up with the ___ trick, which chains reliably.
>>
>> The same issue exists with the variable name: if there are two instances
>> of @local_err, and rule1 matches only one of them, then we nevertheless
>> apply the rules chained to rule1 to both.  Can also be wrong.
>>
>> What are the conditions for "wrong"?
>>
>> Because the ___ chaining is reliable, we know rule1 matched the
>> function, i.e. it has a parameter Error **errp, and it has a automatic
>> variable Error *local_err = NULL.
>>
>> We're good as long as *all* identifiers @local_err in this function are
>> declared that way.  This seems quite likely.  It's not certain, though.
>>
>> Since nested declarations of Error ** variables are rare, we can rely on
>> review to ensure we transform these functions correctly.
>>
>>> So, dropping inheritance in check-rules makes sence, as it may match
>>> (and warn) more interesting cases.
>>>
>>> But for other rules, I'd prefere to be safer, and explictly inherit all
>>> actually inherited identifiers..
>>
>> I still can't see what chaining by function name in addition to the ___
>> chaining buys us.
>
> I'll check this thing soon. And resend today.
>
>>
>>>                                   Still, I feel, we'll never be
>>> absolutely safe with coccinelle :)
>>
>> Right, although this particular problem is not really Coccinelle's
>> fault.  Blindly treating all instances of a certain identifier in a
>> certain area the same regardless of how they are bound to declarations
>> is fundamentally unreliable, regardless of your actual tooling.
>>
>
> Yes, still interesting, can coccinelle do more smart inheritance to match
> exactly same object... I think, I need to CC coccinelle mailing list
> to the next version

I'love to get taught how to chain reliably.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel