[Cocci] Qualifiers for field types get lost

[Cocci] Qualifiers for field types get lost

[Michael Stefaniuc]

[-- Attachment #1: Type: text/plain, Size: 827 bytes --]

Hello,

I have this trivial script to remove useless casts to self:
@ disable drop_cast @
type T;
T E;
@@
- (T)
     E

It works but I'm hitting false positives when the code casts away
qualifiers for field types. Please see the attached test case:
- Good: Cast is kept in foo()
- Bad: Cast is dropped in baz()

Applying the generated diff will lead to a gcc warning:
$ gcc -c  qualifier.c
qualifier.c: In function ‘baz’:
qualifier.c:12:12: warning: return discards ‘const’ qualifier from
pointer target type [-Wdiscarded-qualifiers]
     return &b->i;
            ^~~~~

I tried disabling the optional_qualifier iso but that has no effect:
- Not needed for the foo() case
- No effect for the baz() case

I even tried prepending an alternation with "const T good;" but the cast
still gets removed.

thanks
bye
	michael

[-- Attachment #2: qualifier.c --]
[-- Type: text/x-csrc, Size: 136 bytes --]

int *foo(const int *i)
{
    return (int *)i;
}

struct bar {
    int i;
};

int *baz(const struct bar *b)
{
    return (int *)&b->i;
}

[-- Attachment #3: qualifier.cocci --]
[-- Type: text/plain, Size: 51 bytes --]

@ disable drop_cast @
type T;
T E;
@@
- (T)
     E

[-- Attachment #4: qualifier.res --]
[-- Type: chemical/x-shelx, Size: 175 bytes --]

[-- Attachment #5: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Qualifiers for field types get lost

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1192 bytes --]



On Thu, 14 Feb 2019, Michael Stefaniuc wrote:

> Hello,
>
> I have this trivial script to remove useless casts to self:
> @ disable drop_cast @
> type T;
> T E;
> @@
> - (T)
>      E
>
> It works but I'm hitting false positives when the code casts away
> qualifiers for field types. Please see the attached test case:
> - Good: Cast is kept in foo()
> - Bad: Cast is dropped in baz()
>
> Applying the generated diff will lead to a gcc warning:
> $ gcc -c  qualifier.c
> qualifier.c: In function ‘baz’:
> qualifier.c:12:12: warning: return discards ‘const’ qualifier from
> pointer target type [-Wdiscarded-qualifiers]
>      return &b->i;
>             ^~~~~
>
> I tried disabling the optional_qualifier iso but that has no effect:
> - Not needed for the foo() case
> - No effect for the baz() case
>
> I even tried prepending an alternation with "const T good;" but the cast
> still gets removed.

The following is better, but not perfect, in that it requires that the
const thing have type pointer to a structure.  It could be possible to
explicitly consider more type patterns.

@disable drop_cast@
type T,T1;
T E;
identifier i;
const struct i *c;
@@

(
  c
|
- (T)
   E
)

julia

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Qualifiers for field types get lost

[Michael Stefaniuc]

On 2/14/19 10:38 PM, Julia Lawall wrote:
> 
> 
> On Thu, 14 Feb 2019, Michael Stefaniuc wrote:
> 
>> Hello,
>>
>> I have this trivial script to remove useless casts to self:
>> @ disable drop_cast @
>> type T;
>> T E;
>> @@
>> - (T)
>>      E
>>
>> It works but I'm hitting false positives when the code casts away
>> qualifiers for field types. Please see the attached test case:
>> - Good: Cast is kept in foo()
>> - Bad: Cast is dropped in baz()
>>
>> Applying the generated diff will lead to a gcc warning:
>> $ gcc -c  qualifier.c
>> qualifier.c: In function ‘baz’:
>> qualifier.c:12:12: warning: return discards ‘const’ qualifier from
>> pointer target type [-Wdiscarded-qualifiers]
>>      return &b->i;
>>             ^~~~~
>>
>> I tried disabling the optional_qualifier iso but that has no effect:
>> - Not needed for the foo() case
>> - No effect for the baz() case
>>
>> I even tried prepending an alternation with "const T good;" but the cast
>> still gets removed.
> 
> The following is better, but not perfect, in that it requires that the
> const thing have type pointer to a structure.  It could be possible to
> explicitly consider more type patterns.
Well, it doesn't fixes the problem that the qualifier is not propagated
from the struct to its fields. Code like this isn't matched with or
without the cocci modification.

const int *zzz(const struct bar *b)
{
    return (const int *)&b->i;
}

That's because "&b->i" has the type of just "int*" instead of the
correct "const int*".

bye
	michael

> 
> @disable drop_cast@
> type T,T1;
> T E;
> identifier i;
> const struct i *c;
> @@
> 
> (
>   c
> |
> - (T)
>    E
> )
> 
> julia
> 

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Qualifiers for field types get lost

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 2024 bytes --]



On Thu, 14 Feb 2019, Michael Stefaniuc wrote:

> On 2/14/19 10:38 PM, Julia Lawall wrote:
> >
> >
> > On Thu, 14 Feb 2019, Michael Stefaniuc wrote:
> >
> >> Hello,
> >>
> >> I have this trivial script to remove useless casts to self:
> >> @ disable drop_cast @
> >> type T;
> >> T E;
> >> @@
> >> - (T)
> >>      E
> >>
> >> It works but I'm hitting false positives when the code casts away
> >> qualifiers for field types. Please see the attached test case:
> >> - Good: Cast is kept in foo()
> >> - Bad: Cast is dropped in baz()
> >>
> >> Applying the generated diff will lead to a gcc warning:
> >> $ gcc -c  qualifier.c
> >> qualifier.c: In function ‘baz’:
> >> qualifier.c:12:12: warning: return discards ‘const’ qualifier from
> >> pointer target type [-Wdiscarded-qualifiers]
> >>      return &b->i;
> >>             ^~~~~
> >>
> >> I tried disabling the optional_qualifier iso but that has no effect:
> >> - Not needed for the foo() case
> >> - No effect for the baz() case
> >>
> >> I even tried prepending an alternation with "const T good;" but the cast
> >> still gets removed.
> >
> > The following is better, but not perfect, in that it requires that the
> > const thing have type pointer to a structure.  It could be possible to
> > explicitly consider more type patterns.
> Well, it doesn't fixes the problem that the qualifier is not propagated
> from the struct to its fields. Code like this isn't matched with or
> without the cocci modification.
>
> const int *zzz(const struct bar *b)
> {
>     return (const int *)&b->i;
> }
>
> That's because "&b->i" has the type of just "int*" instead of the
> correct "const int*".

OK.  I can change it in the way that is suggested.  In the short term, you
can add another case to the disjunction, ie - (const T), to reduce the
number of false positives.

julia

>
> bye
> 	michael
>
> >
> > @disable drop_cast@
> > type T,T1;
> > T E;
> > identifier i;
> > const struct i *c;
> > @@
> >
> > (
> >   c
> > |
> > - (T)
> >    E
> > )
> >
> > julia
> >
>
>

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Qualifiers for field types get lost

[Markus Elfring]

> I have this trivial script to remove useless casts to self:
> @ disable drop_cast @
> type T;
> T E;
> @@
> - (T)
>      E
>
> It works but I'm hitting false positives when the code casts away
> qualifiers for field types.

Can this result be expected when your source code transformation
approach is just very generic?


> Applying the generated diff will lead to a gcc warning:
> $ gcc -c  qualifier.c
> qualifier.c: In function ‘baz’:
> qualifier.c:12:12: warning: return discards ‘const’ qualifier from
> pointer target type [-Wdiscarded-qualifiers]
>      return &b->i;
>             ^~~~~


Would you like to add any SmPL filter specifications for such qualifiers?


> I even tried prepending an alternation with "const T good;" but the cast
> still gets removed.

* How do you think about to check the function parameter properties
  in more detail?

* Under which circumstances would you try to cast constness away?

* Would you like to distinguish the involved data types any more?

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Qualifiers for field types get lost

[Michael Stefaniuc]

On 2/15/19 7:30 AM, Markus Elfring wrote:
>> I have this trivial script to remove useless casts to self:
>> @ disable drop_cast @
>> type T;
>> T E;
>> @@
>> - (T)
>>      E
>>
>> It works but I'm hitting false positives when the code casts away
>> qualifiers for field types.
> 
> Can this result be expected when your source code transformation
> approach is just very generic?
I know of no case where removing the self cast in
    (typeof(expr))expr
wouldn't be a true no-op.


>> Applying the generated diff will lead to a gcc warning:
>> $ gcc -c  qualifier.c
>> qualifier.c: In function ‘baz’:
>> qualifier.c:12:12: warning: return discards ‘const’ qualifier from
>> pointer target type [-Wdiscarded-qualifiers]
>>      return &b->i;
>>             ^~~~~
> 
> 
> Would you like to add any SmPL filter specifications for such qualifiers?
No.

>> I even tried prepending an alternation with "const T good;" but the cast
>> still gets removed.
> 
> * How do you think about to check the function parameter properties
>   in more detail?
Badly.

> * Under which circumstances would you try to cast constness away?
I'm not trying, the code does that already.

> * Would you like to distinguish the involved data types any more?
No.

The generic transformation is simple but perfect at the same time and
showcases coccinelles expressiveness and beauty.


bye
	michael



_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Qualifiers for field types get lost

[Michael Stefaniuc]

On 2/15/19 7:22 AM, Julia Lawall wrote:
> 
> 
> On Thu, 14 Feb 2019, Michael Stefaniuc wrote:
> 
>> On 2/14/19 10:38 PM, Julia Lawall wrote:
>>>
>>>
>>> On Thu, 14 Feb 2019, Michael Stefaniuc wrote:
>>>
>>>> Hello,
>>>>
>>>> I have this trivial script to remove useless casts to self:
>>>> @ disable drop_cast @
>>>> type T;
>>>> T E;
>>>> @@
>>>> - (T)
>>>>      E
>>>>
>>>> It works but I'm hitting false positives when the code casts away
>>>> qualifiers for field types. Please see the attached test case:
>>>> - Good: Cast is kept in foo()
>>>> - Bad: Cast is dropped in baz()
>>>>
>>>> Applying the generated diff will lead to a gcc warning:
>>>> $ gcc -c  qualifier.c
>>>> qualifier.c: In function ‘baz’:
>>>> qualifier.c:12:12: warning: return discards ‘const’ qualifier from
>>>> pointer target type [-Wdiscarded-qualifiers]
>>>>      return &b->i;
>>>>             ^~~~~
>>>>
>>>> I tried disabling the optional_qualifier iso but that has no effect:
>>>> - Not needed for the foo() case
>>>> - No effect for the baz() case
>>>>
>>>> I even tried prepending an alternation with "const T good;" but the cast
>>>> still gets removed.
>>>
>>> The following is better, but not perfect, in that it requires that the
>>> const thing have type pointer to a structure.  It could be possible to
>>> explicitly consider more type patterns.
>> Well, it doesn't fixes the problem that the qualifier is not propagated
>> from the struct to its fields. Code like this isn't matched with or
>> without the cocci modification.
>>
>> const int *zzz(const struct bar *b)
>> {
>>     return (const int *)&b->i;
>> }
>>
>> That's because "&b->i" has the type of just "int*" instead of the
>> correct "const int*".
> 
> OK.  I can change it in the way that is suggested.  In the short term, you
> can add another case to the disjunction, ie - (const T), to reduce the
> number of false positives.
Thanks!
I just wanted to report the bug, there's no urgency whatsoever.
After all this is just janitorial work, I'm sure the casts will wait for
me a few more years ;)

bye
	michael



>>>
>>> @disable drop_cast@
>>> type T,T1;
>>> T E;
>>> identifier i;
>>> const struct i *c;
>>> @@
>>>
>>> (
>>>   c
>>> |
>>> - (T)
>>>    E
>>> )
>>>
>>> julia
>>>
>>
>>
> 

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Qualifiers for field types get lost

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1002 bytes --]



On Thu, 14 Feb 2019, Michael Stefaniuc wrote:

> Hello,
>
> I have this trivial script to remove useless casts to self:
> @ disable drop_cast @
> type T;
> T E;
> @@
> - (T)
>      E
>
> It works but I'm hitting false positives when the code casts away
> qualifiers for field types. Please see the attached test case:
> - Good: Cast is kept in foo()
> - Bad: Cast is dropped in baz()
>
> Applying the generated diff will lead to a gcc warning:
> $ gcc -c  qualifier.c
> qualifier.c: In function ‘baz’:
> qualifier.c:12:12: warning: return discards ‘const’ qualifier from
> pointer target type [-Wdiscarded-qualifiers]
>      return &b->i;
>             ^~~~~
>
> I tried disabling the optional_qualifier iso but that has no effect:
> - Not needed for the foo() case
> - No effect for the baz() case
>
> I even tried prepending an alternation with "const T good;" but the cast
> still gets removed.

The const is now propagated to the fields.  Let me know if there are
further problems.

julia

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Qualifiers for field types get lost

[Michael Stefaniuc]

[-- Attachment #1: Type: text/plain, Size: 1581 bytes --]

On 2/15/19 10:04 PM, Julia Lawall wrote:
> 
> 
> On Thu, 14 Feb 2019, Michael Stefaniuc wrote:
> 
>> Hello,
>>
>> I have this trivial script to remove useless casts to self:
>> @ disable drop_cast @
>> type T;
>> T E;
>> @@
>> - (T)
>>      E
>>
>> It works but I'm hitting false positives when the code casts away
>> qualifiers for field types. Please see the attached test case:
>> - Good: Cast is kept in foo()
>> - Bad: Cast is dropped in baz()
>>
>> Applying the generated diff will lead to a gcc warning:
>> $ gcc -c  qualifier.c
>> qualifier.c: In function ‘baz’:
>> qualifier.c:12:12: warning: return discards ‘const’ qualifier from
>> pointer target type [-Wdiscarded-qualifiers]
>>      return &b->i;
>>             ^~~~~
>>
>> I tried disabling the optional_qualifier iso but that has no effect:
>> - Not needed for the foo() case
>> - No effect for the baz() case
>>
>> I even tried prepending an alternation with "const T good;" but the cast
>> still gets removed.
> 
> The const is now propagated to the fields.  Let me know if there are
> further problems.
Many thanks!

I tested it and it removed all but 1 false positive.
I have also validated that it removed only false positives.

There's a qualifier propagation issue left in the case the field is an
array. I have attached a patch that extends the tests/qualifier.c test
case to demonstrate the problem.

But actually I am happy about that leftover false positive as that gave
me a new idea of how to remove some more casts:
https://source.winehq.org/patches/data/158631


Thanks again!
bye
	michael



[-- Attachment #2: 0001-Expand-the-qualifier-propagation-test-with-array-fie.patch --]
[-- Type: text/x-patch, Size: 905 bytes --]

From 7000ce04c9e11a1815783a93dbe9082d22b53a30 Mon Sep 17 00:00:00 2001
From: Michael Stefaniuc <mstefani@mykolab.com>
Date: Fri, 15 Feb 2019 23:09:18 +0100
Subject: [PATCH] Expand the qualifier propagation test with array fields

Signed-off-by: Michael Stefaniuc <mstefani@mykolab.com>
---
 tests/qualifier.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tests/qualifier.c b/tests/qualifier.c
index 05af05f1..149f67f3 100644
--- a/tests/qualifier.c
+++ b/tests/qualifier.c
@@ -10,14 +10,21 @@ int *foo2(int *i)
 
 struct bar {
     int i;
+    int j[2];
 };
 
 int *baz(const struct bar *b)
 {
-    return (int *)&b->i;
+    if (b->i)
+        return (int *)&b->i;
+    else
+        return (int *)&b->j[0];
 }
 
 int *baz2(struct bar *b)
 {
-    return (int *)&b->i;
+    if (b->i)
+        return (int *)&b->i;
+    else
+        return (int *)&b->j[0];
 }
-- 
2.20.1


[-- Attachment #3: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Qualifiers for field types get lost

[Markus Elfring]

> The generic transformation is simple but perfect at the same time and
> showcases coccinelles expressiveness and beauty.

Another “false positive leftover” pointed further software development
opportunities out according to your suggestion “[PATCH] oleaut32/tests:
Propagate the const instead of casting it away”, didn't it?
https://source.winehq.org/patches/data/158631

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Qualifiers for field types get lost

[Markus Elfring]

> There's a qualifier propagation issue left in the case the field is an array.
> I have attached a patch that extends the tests/qualifier.c test case
> to demonstrate the problem.

Can the desired functionality check be performed also with a bit less
source code by using the conditional operator?

-    return (int *)&b->i;
+    return b->i ? (int *)&b->i : (int *)&b->j[0];


Will both test approaches be eventually needed?

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Qualifiers for field types get lost

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1780 bytes --]



On Fri, 15 Feb 2019, Michael Stefaniuc wrote:

> On 2/15/19 10:04 PM, Julia Lawall wrote:
> >
> >
> > On Thu, 14 Feb 2019, Michael Stefaniuc wrote:
> >
> >> Hello,
> >>
> >> I have this trivial script to remove useless casts to self:
> >> @ disable drop_cast @
> >> type T;
> >> T E;
> >> @@
> >> - (T)
> >>      E
> >>
> >> It works but I'm hitting false positives when the code casts away
> >> qualifiers for field types. Please see the attached test case:
> >> - Good: Cast is kept in foo()
> >> - Bad: Cast is dropped in baz()
> >>
> >> Applying the generated diff will lead to a gcc warning:
> >> $ gcc -c  qualifier.c
> >> qualifier.c: In function ‘baz’:
> >> qualifier.c:12:12: warning: return discards ‘const’ qualifier from
> >> pointer target type [-Wdiscarded-qualifiers]
> >>      return &b->i;
> >>             ^~~~~
> >>
> >> I tried disabling the optional_qualifier iso but that has no effect:
> >> - Not needed for the foo() case
> >> - No effect for the baz() case
> >>
> >> I even tried prepending an alternation with "const T good;" but the cast
> >> still gets removed.
> >
> > The const is now propagated to the fields.  Let me know if there are
> > further problems.
> Many thanks!
>
> I tested it and it removed all but 1 false positive.
> I have also validated that it removed only false positives.
>
> There's a qualifier propagation issue left in the case the field is an
> array. I have attached a patch that extends the tests/qualifier.c test
> case to demonstrate the problem.

Thanks for the report.  I will add that case.

> But actually I am happy about that leftover false positive as that gave
> me a new idea of how to remove some more casts:
> https://source.winehq.org/patches/data/158631

Indeed, this looks like a good idea.

julia

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Qualifiers for field types get lost

[Julia Lawall]

I have fixed the array issue.

julia
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Qualifiers for field types get lost

[Michael Stefaniuc]

On 2/16/19 7:33 AM, Markus Elfring wrote:
>> There's a qualifier propagation issue left in the case the field is an array.
>> I have attached a patch that extends the tests/qualifier.c test case
>> to demonstrate the problem.
> 
> Can the desired functionality check be performed also with a bit less
> source code by using the conditional operator?
Sure it can. Would it be better? No.
Exercise left to the interested reader why I consider my variant to be
better.

> 
> -    return (int *)&b->i;
> +    return b->i ? (int *)&b->i : (int *)&b->j[0];
> 
> 
> Will both test approaches be eventually needed?

bye
	michael
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Qualifiers for field types get lost

[Michael Stefaniuc]

On 2/16/19 7:00 AM, Markus Elfring wrote:
>> The generic transformation is simple but perfect at the same time and
>> showcases coccinelles expressiveness and beauty.
> 
> Another “false positive leftover” pointed further software development
> opportunities out according to your suggestion “[PATCH] oleaut32/tests:
> Propagate the const instead of casting it away”, didn't it?
> https://source.winehq.org/patches/data/158631
???

a.) That patch is not a cast to self so no "further software development
opportunities" to my selfcast.cocci script.

b.) Actually doing code cleanup is a better way of finding new
"opportunities" for more cleanup. The vague hints for "further software
development opportunities" are actually harmful as they add noise and
detract from the bug report.

bye
	michael
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Qualifiers for field types get lost

[Michael Stefaniuc]

On 2/16/19 8:20 AM, Julia Lawall wrote:
> I have fixed the array issue.
Thanks! Tested it and it looks good.

bye
	michael

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Qualifiers for field types get lost

[Julia Lawall]

On Sat, 16 Feb 2019, Michael Stefaniuc wrote:

> On 2/16/19 8:20 AM, Julia Lawall wrote:
> > I have fixed the array issue.
> Thanks! Tested it and it looks good.

Great!  Thanks for the report.  The improvement could be useful for other
type checking things.

julia
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci