[PATCH] fix expansion of constant bitfield dereference

[PATCH] fix expansion of constant bitfield dereference

[Luc Van Oostenryck]

During the expansion of a dereference, it's if the initializer
which corrrespond to the offset we're interested is a constant.
In which case this dereference can be avoided and the value
given in the initializer can be used instead.

However, it's not enough to check for the offset since for bitfields
several are placed at the same offset.
Currently, the first initializer matching the offset is selected.

Fix this by refusing such expansion if the constant value correspond
to a bitfield.

Reported-by: Dibyendu Majumdar <mobile@majumdar.org.uk>
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 expand.c                           |  2 ++
 validation/bitfield-expand-deref.c | 27 +++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)
 create mode 100644 validation/bitfield-expand-deref.c

diff --git a/expand.c b/expand.c
index f436b5b50..d44cfac73 100644
--- a/expand.c
+++ b/expand.c
@@ -644,6 +644,8 @@ static int expand_dereference(struct expression *expr)
 		if (value) {
 			/* FIXME! We should check that the size is right! */
 			if (value->type == EXPR_VALUE) {
+				if (is_bitfield_type(value->ctype))
+					return UNSAFE;
 				expr->type = EXPR_VALUE;
 				expr->value = value->value;
 				expr->taint = 0;
diff --git a/validation/bitfield-expand-deref.c b/validation/bitfield-expand-deref.c
new file mode 100644
index 000000000..58d1fe176
--- /dev/null
+++ b/validation/bitfield-expand-deref.c
@@ -0,0 +1,27 @@
+struct s {
+	int a:8;
+	int b:8;
+};
+
+int foo(void)
+{
+	struct s x = { .a = 12, .b = 34, };
+
+	return x.b;
+}
+
+int bar(int a)
+{
+	struct s x = { .a = 12, .b = a, };
+
+	return x.b;
+}
+
+/*
+ * check-name: bitfield expand deref
+ * check-command: test-linearize -Wno-decl $file
+
+ * check-output-ignore
+ * check-output-excludes: ret\..*\$12
+ * check-output-contains: ret\..*\$34
+ */
-- 
2.14.0

Re: [PATCH] fix expansion of constant bitfield dereference

[Dibyendu Majumdar]

Hi Luc,

On 20 August 2017 at 23:16, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> During the expansion of a dereference, it's if the initializer
> which corrrespond to the offset we're interested is a constant.
> In which case this dereference can be avoided and the value
> given in the initializer can be used instead.
>
> However, it's not enough to check for the offset since for bitfields
> several are placed at the same offset.
> Currently, the first initializer matching the offset is selected.
>
> Fix this by refusing such expansion if the constant value correspond
> to a bitfield.
>

Thanks very much for the fix!

Regards
Dibyendu

Re: [PATCH] fix expansion of constant bitfield dereference

[Christopher Li]

On Sun, Aug 20, 2017 at 6:16 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> During the expansion of a dereference, it's if the initializer
> which corrrespond to the offset we're interested is a constant.
> In which case this dereference can be avoided and the value
> given in the initializer can be used instead.
>
> However, it's not enough to check for the offset since for bitfields
> several are placed at the same offset.
> Currently, the first initializer matching the offset is selected.
>
> Fix this by refusing such expansion if the constant value correspond
> to a bitfield.
>

With this applied, sparse still have a bug at did not match the
value to the bit field member, right? I saw on the other email
thread said the value always pick offset zero.

Should I apply this as patch format or wait for it show up in a
git pull request later?


> --- a/expand.c
> +++ b/expand.c
> @@ -644,6 +644,8 @@ static int expand_dereference(struct expression *expr)
>                 if (value) {
>                         /* FIXME! We should check that the size is right! */
>                         if (value->type == EXPR_VALUE) {
> +                               if (is_bitfield_type(value->ctype))
> +                                       return UNSAFE;

You might want to consider move this outside of the EXPR_VALUE.
I assume there is a bug in sparse matching the value to the member
wrong, it could happen to EXPR_FVALUE as well.

Chris

Re: [PATCH] fix expansion of constant bitfield dereference

[Dibyendu Majumdar]

Hi Chris,

On 21 August 2017 at 13:26, Christopher Li <sparse@chrisli.org> wrote:
> On Sun, Aug 20, 2017 at 6:16 PM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
>> During the expansion of a dereference, it's if the initializer
>> which corrrespond to the offset we're interested is a constant.
>> In which case this dereference can be avoided and the value
>> given in the initializer can be used instead.
>>
>> However, it's not enough to check for the offset since for bitfields
>> several are placed at the same offset.
>> Currently, the first initializer matching the offset is selected.
>>
>> Fix this by refusing such expansion if the constant value correspond
>> to a bitfield.
>>
>
>> --- a/expand.c
>> +++ b/expand.c
>> @@ -644,6 +644,8 @@ static int expand_dereference(struct expression *expr)
>>                 if (value) {
>>                         /* FIXME! We should check that the size is right! */
>>                         if (value->type == EXPR_VALUE) {
>> +                               if (is_bitfield_type(value->ctype))
>> +                                       return UNSAFE;
>
> You might want to consider move this outside of the EXPR_VALUE.
> I assume there is a bug in sparse matching the value to the member
> wrong, it could happen to EXPR_FVALUE as well.
>

Since a bitfield is always integer and this issue is occurring with
bitfields, then I guess this change is correct?

Regards
Dibyendu

Re: [PATCH] fix expansion of constant bitfield dereference

[Christopher Li]

On Mon, Aug 21, 2017 at 9:32 AM, Dibyendu Majumdar
<mobile@majumdar.org.uk> wrote:
>> You might want to consider move this outside of the EXPR_VALUE.
>> I assume there is a bug in sparse matching the value to the member
>> wrong, it could happen to EXPR_FVALUE as well.
>>
>
> Since a bitfield is always integer and this issue is occurring with
> bitfields, then I guess this change is correct?

Ok, I forget about that. You are absolutely. right.

Never mind that comment then.

Chris

Re: [PATCH] fix expansion of constant bitfield dereference

[Luc Van Oostenryck]

On Mon, Aug 21, 2017 at 2:26 PM, Christopher Li <sparse@chrisli.org> wrote:
> On Sun, Aug 20, 2017 at 6:16 PM, Luc Van Oostenryck wrote:
>
> With this applied, sparse still have a bug at did not match the
> value to the bit field member, right? I saw on the other email
> thread said the value always pick offset zero.

I'm not sure to understand.
Have you an example?

> Should I apply this as patch format or wait for it show up in a
> git pull request later?

Wait for the pull request, please.

>> --- a/expand.c
>> +++ b/expand.c
>> @@ -644,6 +644,8 @@ static int expand_dereference(struct expression *expr)
>>                 if (value) {
>>                         /* FIXME! We should check that the size is right! */
>>                         if (value->type == EXPR_VALUE) {
>> +                               if (is_bitfield_type(value->ctype))
>> +                                       return UNSAFE;
>
> You might want to consider move this outside of the EXPR_VALUE.
> I assume there is a bug in sparse matching the value to the member
> wrong, it could happen to EXPR_FVALUE as well.

Well ..., if we have an EXPR_FVALUE which type is a bitfield, yes
for sure there would be a bug. But I have no reason to belive there
is such bug and since checking the ctype is more costly than checking
the expr->type, I think it's best so.

Note, I find this check already annoying and hackish if not worse.

-- Luc

Re: [PATCH] fix expansion of constant bitfield dereference

[Christopher Li]

On Mon, Aug 21, 2017 at 9:42 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> On Mon, Aug 21, 2017 at 2:26 PM, Christopher Li <sparse@chrisli.org> wrote:
>> On Sun, Aug 20, 2017 at 6:16 PM, Luc Van Oostenryck wrote:
>>
>> With this applied, sparse still have a bug at did not match the
>> value to the bit field member, right? I saw on the other email
>> thread said the value always pick offset zero.
>
> I'm not sure to understand.
> Have you an example?

I am referenitng to your email reply to Dibyendu:

===========================
It's a very surprising bug. It's not a linearization or
an optimization bug as the AST is already wrong.
With a simpler test case, like:
        struct s {
                char a:4;
                char b:4;
        };

        int foo(void)
        {
                struct s x = { .a = 2, .b = 4 };

                return x.b;
        }

you can see that the linearization produce correct
code for the initializer.
You can also see that the return statement to be
linearized is something like
        STMT_RETURN
                ret_value: EXPR_VALUE (value = 2)

===========================


>
> Wait for the pull request, please.
>

Sure. Glad that I asked :-)
>
> Well ..., if we have an EXPR_FVALUE which type is a bitfield, yes
> for sure there would be a bug. But I have no reason to belive there
> is such bug and since checking the ctype is more costly than checking
> the expr->type, I think it's best so.
>
> Note, I find this check already annoying and hackish if not worse.
>

Yes, from your other email said there is a bug some where else
in sparse yet. (the text I quote you).

Chris

Re: [PATCH] fix expansion of constant bitfield dereference

[Luc Van Oostenryck]

On Mon, Aug 21, 2017 at 6:06 PM, Christopher Li <sparse@chrisli.org> wrote:
>
> I am referenitng to your email reply to Dibyendu:
>
> ===========================
> It's a very surprising bug. It's not a linearization or
> an optimization bug as the AST is already wrong.
> With a simpler test case, like:
>         struct s {
>                 char a:4;
>                 char b:4;
>         };
>
>         int foo(void)
>         {
>                 struct s x = { .a = 2, .b = 4 };
>
>                 return x.b;
>         }
>
> you can see that the linearization produce correct
> code for the initializer.
> You can also see that the return statement to be
> linearized is something like
>         STMT_RETURN
>                 ret_value: EXPR_VALUE (value = 2)


It's precisely this bug that this patch fixes.

>
> Yes, from your other email said there is a bug some where else
> in sparse yet. (the text I quote you).

-- Luc

Re: [PATCH] fix expansion of constant bitfield dereference

[Christopher Li]

On Mon, Aug 21, 2017 at 12:14 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> On Mon, Aug 21, 2017 at 6:06 PM, Christopher Li <sparse@chrisli.org> wrote:
>>
>> I am referenitng to your email reply to Dibyendu:
>>
>> ===========================
>> It's a very surprising bug. It's not a linearization or
>> an optimization bug as the AST is already wrong.
>> With a simpler test case, like:
>>         struct s {
>>                 char a:4;
>>                 char b:4;
>>         };
>>
>>         int foo(void)
>>         {
>>                 struct s x = { .a = 2, .b = 4 };
>>
>>                 return x.b;
>>         }
>>
>> you can see that the linearization produce correct
>> code for the initializer.
>> You can also see that the return statement to be
>> linearized is something like
>>         STMT_RETURN
>>                 ret_value: EXPR_VALUE (value = 2)
>
>
> It's precisely this bug that this patch fixes.

Oh, I see. I misunderstand what bug the patch fix.
Let me go back to read it again.

Sorry about that.

Thanks for the explain.

Chris