diff mbox

regressions on HEAD

Message ID CANeU7Qkde5Cnm4e831e8nX=NhUueY4zk+pz1PFza3mRDhz6=0A@mail.gmail.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Christopher Li March 2, 2018, 11:03 a.m. UTC
On Thu, Mar 1, 2018 at 1:52 PM, Dibyendu Majumdar
<mobile@majumdar.org.uk> wrote:
> On 1 March 2018 at 20:57, Dibyendu Majumdar <mobile@majumdar.org.uk> wrote:
>> On 27 February 2018 at 01:12, Luc Van Oostenryck
>> <luc.vanoostenryck@gmail.com> wrote:
>>> On Mon, Feb 26, 2018 at 03:36:38PM -0800, Christopher Li wrote:
>>>>
>>>> You said you did not found crash in your first round testing.
>>>> However there is some regression of the IR. Can you share
>>>> the regression test case you found?
>>>
>>> I didn't spend much time at it but a typical small example
>>> of what I've seen is:
>>>         void a(void) {
>>>           long b;
>>>           unsigned c = 0;
>>>           for (;;)
>>>             if (c)
>>>               c = b;
>>>         }
>>>
>>
>> Now where is the value pseudo of size 64 coming from as it is not in
>> the code above?
>>
>
> Okay it seems to come from flow.c in find_dominating_stores() - in the
> call to convert_load_instruction(). Any explanation of what this is
> doing?

That is just the memory operation convert to pseudo.
Sparse find out "b" has no dominating store (uninitialized), so
it just replace it with 0 value.

The 64 bit one is b, the long type.
The 32 bit one is c, the int type.

I already find out the place cause it:
The fix is here:

Chris


    simplify cast of constant using wrong size

    This fix the test case after the merge.
    The constant pseudo need to use the target size.

    Signed-off-by: Christopher Li <sparse@chrisli.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Luc Van Oostenryck March 2, 2018, 6:34 p.m. UTC | #1
On Fri, Mar 02, 2018 at 03:03:10AM -0800, Christopher Li wrote:
> 
> That is just the memory operation convert to pseudo.
> Sparse find out "b" has no dominating store (uninitialized), so
> it just replace it with 0 value.
> 
> The 64 bit one is b, the long type.
> The 32 bit one is c, the int type.
> 
> I already find out the place cause it:
> The fix is here:

Even with this fix I see others regressions.

Also the current top patch (which I saw on the ML) and
whose purpose was to fix the crash, will create a
strange situation with code like:
	struct s {
		int r;
		char	buf[<somenumbercloseto65535>];
	};

	struct s *d, *s;
	...

	*d = *s;

Where pseudo->size will be truncated to 16bit and
won't correspond to the real size of the corresponding
object. I don't think a 64K array is something extraordinary
(of course the current 16bit can be extended to 24bit).

This pseudo-size series need much more care, love and testing.
I'll thus revert it for now and the series can be resubmitted
without precipitation when the problems will have been addressed.

-- Luc
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dibyendu Majumdar March 2, 2018, 6:53 p.m. UTC | #2
On 2 March 2018 at 11:03, Christopher Li <sparse@chrisli.org> wrote:
> On Thu, Mar 1, 2018 at 1:52 PM, Dibyendu Majumdar
> <mobile@majumdar.org.uk> wrote:
>> On 1 March 2018 at 20:57, Dibyendu Majumdar <mobile@majumdar.org.uk> wrote:
>>> On 27 February 2018 at 01:12, Luc Van Oostenryck
>>> <luc.vanoostenryck@gmail.com> wrote:
>>>> On Mon, Feb 26, 2018 at 03:36:38PM -0800, Christopher Li wrote:
>>>>>
>>>>> You said you did not found crash in your first round testing.
>>>>> However there is some regression of the IR. Can you share
>>>>> the regression test case you found?
>>>>
>>>> I didn't spend much time at it but a typical small example
>>>> of what I've seen is:
>>>>         void a(void) {
>>>>           long b;
>>>>           unsigned c = 0;
>>>>           for (;;)
>>>>             if (c)
>>>>               c = b;
>>>>         }
>>>>
>>>
>>> Now where is the value pseudo of size 64 coming from as it is not in
>>> the code above?
>>>
>>
>> Okay it seems to come from flow.c in find_dominating_stores() - in the
>> call to convert_load_instruction(). Any explanation of what this is
>> doing?
>
> That is just the memory operation convert to pseudo.
> Sparse find out "b" has no dominating store (uninitialized), so
> it just replace it with 0 value.
>

Does that make sense? I guess that if a var is uninitialized then the
compiler is free to do what it likes, but in the previous version
without size, this pseudo value will be considered 'equivalent' to any
other pseudo value of 0 right? Is that correct?

> The 64 bit one is b, the long type.
> The 32 bit one is c, the int type.
>
> I already find out the place cause it:
> The fix is here:

Okay cool. However I was looking at a recent change that was done
(last year) I think - to use a store to initialize an aggregate using
value_pseudo() - see linearize_one_symbol() in linearize.c. Does that
mean in the pre-sized code, such values would be considered same as
everything else?

In my view the aggregate initialization should be done using a
dedicated OP code and not using store.

But overall, I feel that we don't really care about sizes that are not
'register' size - do we? In reality we probably only care about 8, 16,
32, 64 bits. So one option is to only create distinct pesudo values
when the size is one of these - otherwise create pseudo values of 0
size.

However, that still leaves a question in my mind:

a) Is it okay to assign a value 0 to uninitialized vars?
b) If size is not present or is 0 - does that mean that the CSE logic
will consider all these equivalent? That doesn't sound right to me.

I don't really understand how the CSE handles these but maybe - if you
agree with above then CSE should ignore any pseudo value that is size
0?

Thanks and Regards
Dibyendu
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luc Van Oostenryck March 2, 2018, 8:48 p.m. UTC | #3
On Fri, Mar 02, 2018 at 06:53:49PM +0000, Dibyendu Majumdar wrote:
> On 2 March 2018 at 11:03, Christopher Li <sparse@chrisli.org> wrote:
> >
> > That is just the memory operation convert to pseudo.
> > Sparse find out "b" has no dominating store (uninitialized), so
> > it just replace it with 0 value.
> >
> 
> Does that make sense? I guess that if a var is uninitialized then the
> compiler is free to do what it likes,

It make sense but only as far as you don't forget what you're doing.
The logic of this reasoning is:
- accessing an uninitialized var is undefined behaviour (UB)
- by definition after UB anything can happen
- since anything can happen the compiler can choose (to do) anything
- choosing 0 is as good as anything alse

A more correct treatment is:
- use a specific value (UNDEF), unique for each var
- propagate this value as far as possible
- be very carefull with any simplification involving UNDEF
- at the end, issues an appropriate warning and/or replace it
  the UNDEF by some appropriate value (for example 0) if this is desired.

> but in the previous version
> without size, this pseudo value will be considered 'equivalent' to any
> other pseudo value of 0 right? Is that correct?

Indeed.
Anyway, once you have UB anything is 'correct'.
 
> > The 64 bit one is b, the long type.
> > The 32 bit one is c, the int type.
> >
> > I already find out the place cause it:
> > The fix is here:
> 
> Okay cool. However I was looking at a recent change that was done
> (last year) I think - to use a store to initialize an aggregate using
> value_pseudo() - see linearize_one_symbol() in linearize.c. Does that
> mean in the pre-sized code, such values would be considered same as
> everything else?
> 
> In my view the aggregate initialization should be done using a
> dedicated OP code and not using store.

This can be done, but this opcode would be semantically be a store
so it doesn't help to have another op for it.

> But overall, I feel that we don't really care about sizes that are not
> 'register' size - do we? In reality we probably only care about 8, 16,
> 32, 64 bits.

Even is 'pseudo' holds for 'pseudo-register' they are used for any 'values'.

> However, that still leaves a question in my mind:
> 
> a) Is it okay to assign a value 0 to uninitialized vars?

As explained here above, it's OK but it's just a compiler choice
for something UB (and certainly not worse than any other).

> b) If size is not present or is 0 - does that mean that the CSE logic
> will consider all these equivalent? That doesn't sound right to me.

Sure, unless you special case it in CSE.

> I don't really understand how the CSE handles these but maybe - if you
> agree with above then CSE should ignore any pseudo value that is size
> 0?

Currently, a size of zero is used for erroneous code, like incomplete
type and things like that (-1/~0 is also used for that). In such
situations CSE make no sense anyway, we just want to continue parsing
and give the best possible diagnostic for the remaining of the code.

Unitialized var is different because, in full generality, you can't
(always) prove that the var is accessed *before* being initialized.
It's equivalent to the halting problem. It's very common that you
have code with some vars that are not initialized but which receive
a value before the var is used. In other words, the var is
uninitialized in the code but is initialized at run-time.
In short, the compiler can't prove that the access is UB or not.


-- Luc
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dibyendu Majumdar March 2, 2018, 9:06 p.m. UTC | #4
On 2 March 2018 at 20:48, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> On Fri, Mar 02, 2018 at 06:53:49PM +0000, Dibyendu Majumdar wrote:
>> On 2 March 2018 at 11:03, Christopher Li <sparse@chrisli.org> wrote:
>> >
>> > That is just the memory operation convert to pseudo.
>> > Sparse find out "b" has no dominating store (uninitialized), so
>> > it just replace it with 0 value.
>> >
>>
>> Does that make sense? I guess that if a var is uninitialized then the
>> compiler is free to do what it likes,
>
> It make sense but only as far as you don't forget what you're doing.
> The logic of this reasoning is:
> - accessing an uninitialized var is undefined behaviour (UB)
> - by definition after UB anything can happen
> - since anything can happen the compiler can choose (to do) anything
> - choosing 0 is as good as anything alse
>
> A more correct treatment is:
> - use a specific value (UNDEF), unique for each var
> - propagate this value as far as possible
> - be very carefull with any simplification involving UNDEF
> - at the end, issues an appropriate warning and/or replace it
>   the UNDEF by some appropriate value (for example 0) if this is desired.
>

Okay - I guess in my suggestion the size '0' signifies undefined or
not 'register size' therefore not eligible for CSE.

>> but in the previous version
>> without size, this pseudo value will be considered 'equivalent' to any
>> other pseudo value of 0 right? Is that correct?
>
> Indeed.
> Anyway, once you have UB anything is 'correct'.
>
>> > The 64 bit one is b, the long type.
>> > The 32 bit one is c, the int type.
>> >
>> > I already find out the place cause it:
>> > The fix is here:
>>
>> Okay cool. However I was looking at a recent change that was done
>> (last year) I think - to use a store to initialize an aggregate using
>> value_pseudo() - see linearize_one_symbol() in linearize.c. Does that
>> mean in the pre-sized code, such values would be considered same as
>> everything else?
>>
>> In my view the aggregate initialization should be done using a
>> dedicated OP code and not using store.
>
> This can be done, but this opcode would be semantically be a store
> so it doesn't help to have another op for it.

Thinking more about it - perhaps if a pseudo value of size 0 was used
when the aggregate is bigger than register size then this would still
be okay. I can see that when values are register size store makes
sense.

>
>> But overall, I feel that we don't really care about sizes that are not
>> 'register' size - do we? In reality we probably only care about 8, 16,
>> 32, 64 bits.
>
> Even is 'pseudo' holds for 'pseudo-register' they are used for any 'values'.
>

Not sure I understood this point - I was suggesting that we only set
pseudo value size when it makes sense to do so.

>> However, that still leaves a question in my mind:
>>
>> a) Is it okay to assign a value 0 to uninitialized vars?
>
> As explained here above, it's OK but it's just a compiler choice
> for something UB (and certainly not worse than any other).
>
>> b) If size is not present or is 0 - does that mean that the CSE logic
>> will consider all these equivalent? That doesn't sound right to me.
>
> Sure, unless you special case it in CSE.

I was wondering if the source of the bugs related to uninitialized var
and CSE is this that the CSE doesn't know the value came from garbage.

>
>> I don't really understand how the CSE handles these but maybe - if you
>> agree with above then CSE should ignore any pseudo value that is size
>> 0?
>
> Currently, a size of zero is used for erroneous code, like incomplete
> type and things like that (-1/~0 is also used for that). In such
> situations CSE make no sense anyway, we just want to continue parsing
> and give the best possible diagnostic for the remaining of the code.
>

I didn't follow - size 0 is used for pseudo values?

> Unitialized var is different because, in full generality, you can't
> (always) prove that the var is accessed *before* being initialized.
> It's equivalent to the halting problem. It's very common that you
> have code with some vars that are not initialized but which receive
> a value before the var is used. In other words, the var is
> uninitialized in the code but is initialized at run-time.
> In short, the compiler can't prove that the access is UB or not.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christopher Li March 2, 2018, 10:15 p.m. UTC | #5
On Fri, Mar 2, 2018 at 10:34 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> Even with this fix I see others regressions.
>
> Also the current top patch (which I saw on the ML) and
> whose purpose was to fix the crash, will create a
> strange situation with code like:
>         struct s {
>                 int r;
>                 char    buf[<somenumbercloseto65535>];
>         };
>
>         struct s *d, *s;
>         ...
>
>         *d = *s;
>
> Where pseudo->size will be truncated to 16bit and
> won't correspond to the real size of the corresponding
> object. I don't think a 64K array is something extraordinary
> (of course the current 16bit can be extended to 24bit).

Are you kidding me? PSEUDO_VAL only cover what ever
is able to store in side pseudo->value, which is "long long". 16
bit is plenty enough.

struct *d is not freaking going to store in PSEUDO_VAL period.

I don't know what you are smoking. You'd better explain this
pseudo->size going to exceed 16 bit thing if you think that is
trigger-able in the current code.

> This pseudo-size series need much more care, love and testing.
> I'll thus revert it for now and the series can be resubmitted
> without precipitation when the problems will have been addressed.

What test case do you still have? Can you submit some test case
expose the problem rather than talk about there is some problem.
That is FUD.

We should keep it on a branch so others can run  test on it to
expose problems. It is a chicken and egg problem.

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luc Van Oostenryck March 2, 2018, 10:18 p.m. UTC | #6
On Fri, Mar 02, 2018 at 09:06:21PM +0000, Dibyendu Majumdar wrote:
> On 2 March 2018 at 20:48, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> > On Fri, Mar 02, 2018 at 06:53:49PM +0000, Dibyendu Majumdar wrote:
> >> On 2 March 2018 at 11:03, Christopher Li <sparse@chrisli.org> wrote:
> >> >
> >> > That is just the memory operation convert to pseudo.
> >> > Sparse find out "b" has no dominating store (uninitialized), so
> >> > it just replace it with 0 value.
> >> >
> >>
> >> Does that make sense? I guess that if a var is uninitialized then the
> >> compiler is free to do what it likes,
> >
> > It make sense but only as far as you don't forget what you're doing.
> > The logic of this reasoning is:
> > - accessing an uninitialized var is undefined behaviour (UB)
> > - by definition after UB anything can happen
> > - since anything can happen the compiler can choose (to do) anything
> > - choosing 0 is as good as anything alse
> >
> > A more correct treatment is:
> > - use a specific value (UNDEF), unique for each var
> > - propagate this value as far as possible
> > - be very carefull with any simplification involving UNDEF
> > - at the end, issues an appropriate warning and/or replace it
> >   the UNDEF by some appropriate value (for example 0) if this is desired.
> >
> 
> Okay - I guess in my suggestion the size '0' signifies undefined or
> not 'register size' therefore not eligible for CSE.
> 
> >> but in the previous version
> >> without size, this pseudo value will be considered 'equivalent' to any
> >> other pseudo value of 0 right? Is that correct?
> >
> > Indeed.
> > Anyway, once you have UB anything is 'correct'.
> >
> >> > The 64 bit one is b, the long type.
> >> > The 32 bit one is c, the int type.
> >> >
> >> > I already find out the place cause it:
> >> > The fix is here:
> >>
> >> Okay cool. However I was looking at a recent change that was done
> >> (last year) I think - to use a store to initialize an aggregate using
> >> value_pseudo() - see linearize_one_symbol() in linearize.c. Does that
> >> mean in the pre-sized code, such values would be considered same as
> >> everything else?
> >>
> >> In my view the aggregate initialization should be done using a
> >> dedicated OP code and not using store.
> >
> > This can be done, but this opcode would be semantically be a store
> > so it doesn't help to have another op for it.
> 
> Thinking more about it - perhaps if a pseudo value of size 0 was used
> when the aggregate is bigger than register size then this would still
> be okay. I can see that when values are register size store makes
> sense.
> 
> >
> >> But overall, I feel that we don't really care about sizes that are not
> >> 'register' size - do we? In reality we probably only care about 8, 16,
> >> 32, 64 bits.
> >
> > Even is 'pseudo' holds for 'pseudo-register' they are used for any 'values'.
> Not sure I understood this point - I was suggesting that we only set
> pseudo value size when it makes sense to do so.

I meant that I think that it would be dangerous to special-case 
pseudos with register-size and non-register-size (we're by dangerous
I mostly mean 'add unneeded complexity').
It's a fuzzy concept anyway and target specific while there is no need
to be target specific.

> >> However, that still leaves a question in my mind:
> >>
> >> a) Is it okay to assign a value 0 to uninitialized vars?
> >
> > As explained here above, it's OK but it's just a compiler choice
> > for something UB (and certainly not worse than any other).
> >
> >> b) If size is not present or is 0 - does that mean that the CSE logic
> >> will consider all these equivalent? That doesn't sound right to me.
> >
> > Sure, unless you special case it in CSE.
> 
> I was wondering if the source of the bugs related to uninitialized var
> and CSE is this that the CSE doesn't know the value came from garbage.

Once we choose to give value 0 to otherwise unintialized vars, it's
no more garbage than anything else. It's something very sane to do
for a 'robust compiler'.

> >> I don't really understand how the CSE handles these but maybe - if you
> >> agree with above then CSE should ignore any pseudo value that is size
> >> 0?
> >
> > Currently, a size of zero is used for erroneous code, like incomplete
> > type and things like that (-1/~0 is also used for that). In such
> > situations CSE make no sense anyway, we just want to continue parsing
> > and give the best possible diagnostic for the remaining of the code.
> >
> 
> I didn't follow - size 0 is used for pseudo values?

Sorry, when I said 'size' I meant 'size of the underlying type' while
you were talking about the size given to the pseudo.
 
But my answer would be 'yes for pseudos coming from errors', 'no for
pseudo from variables we're not sure if they will be used only after
having received a value', 'why special case at this level? it would
be better to use consistently a special kind of value for erroneous
things (VOID is used for this but also used for other things which
doesn't help)'.

-- Luc
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dibyendu Majumdar March 2, 2018, 10:30 p.m. UTC | #7
On 2 March 2018 at 22:18, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> On Fri, Mar 02, 2018 at 09:06:21PM +0000, Dibyendu Majumdar wrote:
>> On 2 March 2018 at 20:48, Luc Van Oostenryck
>> <luc.vanoostenryck@gmail.com> wrote:
>> >> But overall, I feel that we don't really care about sizes that are not
>> >> 'register' size - do we? In reality we probably only care about 8, 16,
>> >> 32, 64 bits.
>> >
>> > Even is 'pseudo' holds for 'pseudo-register' they are used for any 'values'.
>> Not sure I understood this point - I was suggesting that we only set
>> pseudo value size when it makes sense to do so.
>
> I meant that I think that it would be dangerous to special-case
> pseudos with register-size and non-register-size (we're by dangerous
> I mostly mean 'add unneeded complexity').
> It's a fuzzy concept anyway and target specific while there is no need
> to be target specific.
>

I wasn't talking about pseudos in general - only pseudos of type
PSEUDO_VAL. For such a size that is bigger than 64-bits makes no sense
- do you agree?

>> >> However, that still leaves a question in my mind:
>> >>
>> >> a) Is it okay to assign a value 0 to uninitialized vars?
>> >
>> > As explained here above, it's OK but it's just a compiler choice
>> > for something UB (and certainly not worse than any other).
>> >
>> >> b) If size is not present or is 0 - does that mean that the CSE logic
>> >> will consider all these equivalent? That doesn't sound right to me.
>> >
>> > Sure, unless you special case it in CSE.
>>
>> I was wondering if the source of the bugs related to uninitialized var
>> and CSE is this that the CSE doesn't know the value came from garbage.
>
> Once we choose to give value 0 to otherwise unintialized vars, it's
> no more garbage than anything else. It's something very sane to do
> for a 'robust compiler'.
>

I would prefer that the compiler a) doesn't try to CSE values that are
garbage, and b) it is better to know something is garbage than assign
a value 0. Latter makes the CSE do unexpected things I think. In fact
on detecting that a garbage value is being used somewhere the compiler
should simply stop trying to CSE. In my project, the LLVM backend will
stop compiling as soon as it sees a VOID pseudo.


>> >> I don't really understand how the CSE handles these but maybe - if you
>> >> agree with above then CSE should ignore any pseudo value that is size
>> >> 0?
>> >
>> > Currently, a size of zero is used for erroneous code, like incomplete
>> > type and things like that (-1/~0 is also used for that). In such
>> > situations CSE make no sense anyway, we just want to continue parsing
>> > and give the best possible diagnostic for the remaining of the code.
>> >
>>
>> I didn't follow - size 0 is used for pseudo values?
>
> Sorry, when I said 'size' I meant 'size of the underlying type' while
> you were talking about the size given to the pseudo.

Again - I am not talking about pseudos in general - only those of
PSEUDO_VAL type.

>
> But my answer would be 'yes for pseudos coming from errors', 'no for
> pseudo from variables we're not sure if they will be used only after
> having received a value', 'why special case at this level? it would
> be better to use consistently a special kind of value for erroneous
> things (VOID is used for this but also used for other things which
> doesn't help)'.
>

So for un-initialized vars we should not be creating a value pseudo
with 0 - is that what you are saying?

Regards
Dibyendu
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dibyendu Majumdar March 2, 2018, 10:43 p.m. UTC | #8
On 2 March 2018 at 22:30, Dibyendu Majumdar <mobile@majumdar.org.uk> wrote:
> On 2 March 2018 at 22:18, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
>>> I was wondering if the source of the bugs related to uninitialized var
>>> and CSE is this that the CSE doesn't know the value came from garbage.
>>
>> Once we choose to give value 0 to otherwise unintialized vars, it's
>> no more garbage than anything else. It's something very sane to do
>> for a 'robust compiler'.
>>
>
> I would prefer that the compiler a) doesn't try to CSE values that are
> garbage, and b) it is better to know something is garbage than assign
> a value 0. Latter makes the CSE do unexpected things I think. In fact
> on detecting that a garbage value is being used somewhere the compiler
> should simply stop trying to CSE. In my project, the LLVM backend will
> stop compiling as soon as it sees a VOID pseudo.
>
>

BTW Sparse IR tends to be always correct before CSE even when there
are uninitialized vars. So by stopping CSE when garbage values are
detected we preserve 'correctness' which is far more important than
some erroneous optimization that produces meaningless IR.

Regards
Dibyendu
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luc Van Oostenryck March 2, 2018, 10:46 p.m. UTC | #9
On Fri, Mar 02, 2018 at 02:15:24PM -0800, Christopher Li wrote:
> On Fri, Mar 2, 2018 at 10:34 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> >
> > Even with this fix I see others regressions.
> >
> > Also the current top patch (which I saw on the ML) and
> > whose purpose was to fix the crash, will create a
> > strange situation with code like:
> >         struct s {
> >                 int r;
> >                 char    buf[<somenumbercloseto65535>];
> >         };
> >
> >         struct s *d, *s;
> >         ...
> >
> >         *d = *s;
> >
> > Where pseudo->size will be truncated to 16bit and
> > won't correspond to the real size of the corresponding
> > object. I don't think a 64K array is something extraordinary
> > (of course the current 16bit can be extended to 24bit).
> 
> Are you kidding me? PSEUDO_VAL only cover what ever
> is able to store in side pseudo->value, which is "long long". 16
> bit is plenty enough.
> 
> struct *d is not freaking going to store in PSEUDO_VAL period.
> 
> I don't know what you are smoking. You'd better explain this
> pseudo->size going to exceed 16 bit thing if you think that is
> trigger-able in the current code.
> 
> > This pseudo-size series need much more care, love and testing.
> > I'll thus revert it for now and the series can be resubmitted
> > without precipitation when the problems will have been addressed.
> 
> What test case do you still have? Can you submit some test case
> expose the problem rather than talk about there is some problem.
> That is FUD.

OK.
I tried to be constructive and once more to explain you where
the problems are.
Two days ago Linus talked about the need to trust others, or
at least to need to find someone you can trust. You replied
  "Yes you right" 
and
  "Thanks for the very insightful feed back. That is super helpful."
I had hoped I'll trigger some changes from your side allowing
to move forward. I think I've been too optimistic.
 
Since visibly this trust is not present, I can only invite you
to read the code, understand it and test it by yourself.

-- Luc
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dibyendu Majumdar March 2, 2018, 10:57 p.m. UTC | #10
On 2 March 2018 at 22:46, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:

> I tried to be constructive and once more to explain you where
> the problems are.
> Two days ago Linus talked about the need to trust others, or
> at least to need to find someone you can trust. You replied
>   "Yes you right"
> and
>   "Thanks for the very insightful feed back. That is super helpful."
> I had hoped I'll trigger some changes from your side allowing
> to move forward. I think I've been too optimistic.
>
> Since visibly this trust is not present, I can only invite you
> to read the code, understand it and test it by yourself.
>

Luc, are you saying that your code should not be subject to any review
or QA? I don't see that Chris's ask is unreasonable - most projects do
a Beta / RC release before doing an actual release. Trust does not
imply 'just accept what I give you or else'.

Regards
Dibyendu
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christopher Li March 2, 2018, 11:01 p.m. UTC | #11
On Fri, Mar 2, 2018 at 2:46 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> OK.
> I tried to be constructive and once more to explain you where
> the problems are.
> Two days ago Linus talked about the need to trust others, or
> at least to need to find someone you can trust. You replied
>   "Yes you right"
> and
>   "Thanks for the very insightful feed back. That is super helpful."
> I had hoped I'll trigger some changes from your side allowing
> to move forward. I think I've been too optimistic.

See the other email I send you. I do agree on for release 0.5.2,
this revert is fine. I even suggest have one RC1 for people to test it.

> Since visibly this trust is not present, I can only invite you
> to read the code, understand it and test it by yourself.

We have too much discussion on this already. Let's do this,
put it on a branch, if you have test can break. Let's break it
and I will try to fix it since I introduce it. That is the way it does not
interference the main release. That way we can do more constructive
work instead of keep on arguing it.

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/simplify.c b/simplify.c
index 09931fe..3d32cdd 100644
--- a/simplify.c
+++ b/simplify.c
@@ -950,7 +950,7 @@  static int simplify_cast(struct instruction *insn)
        if (constant(src)) {
                int sign = orig_type->ctype.modifiers & MOD_SIGNED;
                long long val = get_cast_value(src->value, orig_size,
size, sign);
-               src = value_pseudo(orig_type, val);
+               src = value_pseudo(insn->type, val);
                goto simplify;
        }