[PATCH v2 0/2] fix: give a type to bad conditionnal expressions

[PATCH v2 0/2] fix: give a type to bad conditionnal expressions

[Luc Van Oostenryck]

The goal of this series is to fix the slow compilation of
the Wine test file.

It should be safe enough but IMO it's not -rc5 material.


This series is available in the git repository at:

  git://github.com/lucvoo/sparse.git fix-type-bad-cond-expr-v2

for you to fetch changes up to be35da05f38df984d518bc4670b36aa0b7ad4185:

  fix: give a type to bad cond expr with known condition (2017-08-04 15:49:49 +0200)

----------------------------------------------------------------
Luc Van Oostenryck (2):
      take comma expr in account for constant value
      fix: give a type to bad cond expr with known condition

 evaluate.c                   | 12 ++++++++++++
 expand.c                     | 30 ++++++++++++++++++++++++++++++
 expression.h                 |  1 +
 validation/cond-err-expand.c | 27 +++++++++++++++++++++++++++
 4 files changed, 70 insertions(+)
 create mode 100644 validation/cond-err-expand.c

[PATCH v2 1/2] take comma expr in account for constant value

[Luc Van Oostenryck]

It's often the case that we simply need the expr's truth value.

To get the value of an expression or simply if we need to know
if it's a constant value we can use some functions like
get_expression_value() or const_expression_value() depending
on the type of warning needed if the expression is not constant
(for various definition of constant).

However none of these functions take in account the fact that
a comma expression can also have a value known at compile time.

In order to not introduce unwanted change elsewheer in the code,
introduce a new function expr_truth_value() which never warn
but will return -1 if the expr have no known boolean value.

Note: the whole set of functions should be unified to take comma
      expressions in account when needed.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 expand.c     | 30 ++++++++++++++++++++++++++++++
 expression.h |  1 +
 2 files changed, 31 insertions(+)

diff --git a/expand.c b/expand.c
index 5f908c971..f436b5b50 100644
--- a/expand.c
+++ b/expand.c
@@ -1281,6 +1281,36 @@ long long get_expression_value_silent(struct expression *expr)
 	return __get_expression_value(expr, 2);
 }
 
+int expr_truth_value(struct expression *expr)
+{
+	const int saved = conservative;
+	struct symbol *ctype;
+
+	if (!expr)
+		return 0;
+
+	ctype = evaluate_expression(expr);
+	if (!ctype)
+		return -1;
+
+	conservative = 1;
+	expand_expression(expr);
+	conservative = saved;
+
+redo:
+	switch (expr->type) {
+	case EXPR_COMMA:
+		expr = expr->right;
+		goto redo;
+	case EXPR_VALUE:
+		return expr->value != 0;
+	case EXPR_FVALUE:
+		return expr->fvalue != 0;
+	default:
+		return -1;
+	}
+}
+
 int is_zero_constant(struct expression *expr)
 {
 	const int saved = conservative;
diff --git a/expression.h b/expression.h
index 80b3be5f5..d0f3ac925 100644
--- a/expression.h
+++ b/expression.h
@@ -178,6 +178,7 @@ struct expression {
 
 /* Constant expression values */
 int is_zero_constant(struct expression *);
+int expr_truth_value(struct expression *expr);
 long long get_expression_value(struct expression *);
 long long const_expression_value(struct expression *);
 long long get_expression_value_silent(struct expression *expr);
-- 
2.13.2

Re: [PATCH v2 1/2] take comma expr in account for constant value

[Christopher Li]

On Fri, Aug 4, 2017 at 12:06 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> It's often the case that we simply need the expr's truth value.
>
> To get the value of an expression or simply if we need to know
> if it's a constant value we can use some functions like
> get_expression_value() or const_expression_value() depending
> on the type of warning needed if the expression is not constant
> (for various definition of constant).
>
> However none of these functions take in account the fact that
> a comma expression can also have a value known at compile time.
>
> In order to not introduce unwanted change elsewheer in the code,
> introduce a new function expr_truth_value() which never warn
> but will return -1 if the expr have no known boolean value.
>
> Note: the whole set of functions should be unified to take comma
>       expressions in account when needed.
>
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
>  expand.c     | 30 ++++++++++++++++++++++++++++++
>  expression.h |  1 +
>  2 files changed, 31 insertions(+)
>
> diff --git a/expand.c b/expand.c
> index 5f908c971..f436b5b50 100644
> --- a/expand.c
> +++ b/expand.c
> @@ -1281,6 +1281,36 @@ long long get_expression_value_silent(struct expression *expr)
>         return __get_expression_value(expr, 2);
>  }
>
> +int expr_truth_value(struct expression *expr)

Looks fine to me.

One of the very minor note is that. I think this patch can actually merge with
the next one. It is easier to read patch have both the define and usage.
I found myself need to switch between patches to read how it was used.

The test case on the other hand can justify to become a standalone patch.
Some time we want see the how the code change (with or without patches)
impact the test case.

Another minor commend is that, if it is not for RC5.
You actually don't need to worry about code impact range
any more. You might just as well do the full change into the constant
expression value evaluation.

Chris

Re: [PATCH v2 1/2] take comma expr in account for constant value

[Luc Van Oostenryck]

On Fri, Aug 4, 2017 at 9:49 PM, Christopher Li <sparse@chrisli.org> wrote:
> Looks fine to me.
>
> One of the very minor note is that. I think this patch can actually merge with
> the next one. It is easier to read patch have both the define and usage.
> I found myself need to switch between patches to read how it was used.

I really prefer to keep independent things separated (for a lot of reasons)
but if it is a problme for you feel free to squash both together I don't mind
(and have no others series which would depend on this one).

> The test case on the other hand can justify to become a standalone patch.
> Some time we want see the how the code change (with or without patches)
> impact the test case.

Well ... if you had the test case before the fix you break
bissectability and adding it after is pointless ...
We could add it before and tagging it as known-to-fail
and remove the tag when adding the fix but ...

> Another minor commend is that, if it is not for RC5.

Well, it's up to you to decide if it is for -rc5 or not.
Personally, I wouldn't take such a patch for such problem in -rc5
(but this decision would also depend on the frequency of releases,
for example). On one hand, the change seems quite limited and
'obviously correct' (famous last words), on the other hand, every
changes, even the smallest need to retest things and delay this
release a bit more.

> You actually don't need to worry about code impact range
> any more. You might just as well do the full change into the constant
> expression value evaluation.

Well, it's also because I would need to think a bit more about the API
look after the different uses regarding the warning flag and such.
I'm not even convinced that we must in general take in account
comma expressions like done here for this specific case.
All things that would need a bit more thinking and time.

-- Luc

Re: [PATCH v2 1/2] take comma expr in account for constant value

[Christopher Li]

On Fri, Aug 4, 2017 at 4:27 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> I really prefer to keep independent things separated (for a lot of reasons)
> but if it is a problme for you feel free to squash both together I don't mind
> (and have no others series which would depend on this one).

As I said, it is a minor one. I also realize it is some what personal
preference.
It will not impact how the patch get merged or not.

>
>> The test case on the other hand can justify to become a standalone patch.
>> Some time we want see the how the code change (with or without patches)
>> impact the test case.
>
> Well ... if you had the test case before the fix you break
> bissectability and adding it after is pointless ...

Before was the model for a while when Josh was the maintainer.
I did not push separate test patch any more.

> We could add it before and tagging it as known-to-fail
> and remove the tag when adding the fix but ...

May be not justifiable.

>
>> Another minor commend is that, if it is not for RC5.
>
> Well, it's up to you to decide if it is for -rc5 or not.

I will listen to your recommendation as the base line.

If it compress 20 seconds to 2 seconds. I vote for include it.

But really that is your call too. I am fine either way.

> Personally, I wouldn't take such a patch for such problem in -rc5
> (but this decision would also depend on the frequency of releases,
> for example). On one hand, the change seems quite limited and
> 'obviously correct' (famous last words), on the other hand, every
> changes, even the smallest need to retest things and delay this
> release a bit more.

After the RC5 release, we should have some good test on it any way.
Chance of this patch breaking new things is relative low.

> Well, it's also because I would need to think a bit more about the API
> look after the different uses regarding the warning flag and such.
> I'm not even convinced that we must in general take in account
> comma expressions like done here for this specific case.
> All things that would need a bit more thinking and time.

Yes, your call :-)

Chris

Re: [PATCH v2 1/2] take comma expr in account for constant value

[Luc Van Oostenryck]

On Fri, Aug 4, 2017 at 11:05 PM, Christopher Li <sparse@chrisli.org> wrote:
> On Fri, Aug 4, 2017 at 4:27 PM, Luc Van Oostenryck wrote:
> <luc.vanoostenryck@gmail.com> wrote:
>> I really prefer to keep independent things separated (for a lot of reasons)
>> but if it is a problme for you feel free to squash both together I don't mind
>> (and have no others series which would depend on this one).
>
> As I said, it is a minor one. I also realize it is some what personal
> preference.
> It will not impact how the patch get merged or not.

OK.

>>
>>> The test case on the other hand can justify to become a standalone patch.
>>> Some time we want see the how the code change (with or without patches)
>>> impact the test case.
>>
>> Well ... if you had the test case before the fix you break
>> bissectability and adding it after is pointless ...
>
> Before was the model for a while when Josh was the maintainer.
> I did not push separate test patch any more.
>
>> We could add it before and tagging it as known-to-fail
>> and remove the tag when adding the fix but ...
>
> May be not justifiable.
>
>>
>>> Another minor commend is that, if it is not for RC5.
>>
>> Well, it's up to you to decide if it is for -rc5 or not.
>
> I will listen to your recommendation as the base line.
>
> If it compress 20 seconds to 2 seconds. I vote for include it.

If only it would do so for every input code ;)

> But really that is your call too. I am fine either way.
>
>> Personally, I wouldn't take such a patch for such problem in -rc5
>> (but this decision would also depend on the frequency of releases,
>> for example). On one hand, the change seems quite limited and
>> 'obviously correct' (famous last words), on the other hand, every
>> changes, even the smallest need to retest things and delay this
>> release a bit more.
>
> After the RC5 release, we should have some good test on it any way.
> Chance of this patch breaking new things is relative low.

It can only impact code that is in fact erroneous.

>> Well, it's also because I would need to think a bit more about the API
>> look after the different uses regarding the warning flag and such.
>> I'm not even convinced that we must in general take in account
>> comma expressions like done here for this specific case.
>> All things that would need a bit more thinking and time.
>
> Yes, your call :-)

OK, considering also that the situation may be present in other
code (at various degree) and that this non-expansion of the
builtin is really bad I'm fine to include it.

Strangely I have a series of 5 or 6 patches fixing some similar situations
(where some code is not evaluated and/or expanded, often creating
duplicated warnings) but not this precise case.
I never posted them because I considered it was too late in the release ...

-- Luc

Re: [PATCH v2 1/2] take comma expr in account for constant value

[Christopher Li]

On Fri, Aug 4, 2017 at 5:20 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:

>>
>> If it compress 20 seconds to 2 seconds. I vote for include it.
>
> If only it would do so for every input code ;)
>

Because the offending source is come from a system
header file, I think it is more likely to hit in the field than
average project source file.


>
> OK, considering also that the situation may be present in other
> code (at various degree) and that this non-expansion of the
> builtin is really bad I'm fine to include it.
>
> Strangely I have a series of 5 or 6 patches fixing some similar situations
> (where some code is not evaluated and/or expanded, often creating
> duplicated warnings) but not this precise case.
> I never posted them because I considered it was too late in the release ...

I already applied as patch to sparse-next. I am doing the kernel
compile test right now. I will push it soon.

I confirm it cut the offending compile from 23 secons to:

real 0m3.425s
user 0m3.237s
sys 0m0.135s


Chris

[PATCH v2 2/2] fix: give a type to bad cond expr with known condition

[Luc Van Oostenryck]

Conditional expressions whose second & third operands
have non-compatible types are not conform to the C standard
and sparse emit a warning for them and return the expression
as being erroneous. In consequence, such expressions are not
given a type. This, in turn, makes that some further processing
cannot be done (correctly).

It seems that neither GCC nor clang emit a warning when
there is a type mismatch but the condition is a constant.

In the case we're interested here (the slow compilation of a file)
the operation that cannot be done is the expansion its operands.
This, in turn and among other things, makes that builtins like
__builtin_constant_p() are not evaluated with disatrous consequence
for the amount of work done in the next phases.

Fix this by giving to conditional expressions with constant
condition the same type as the operand selected by the conditional
(but keeping the warning) as GCC & clang seems to do.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 evaluate.c                   | 12 ++++++++++++
 validation/cond-err-expand.c | 27 +++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)
 create mode 100644 validation/cond-err-expand.c

diff --git a/evaluate.c b/evaluate.c
index cf3cf244d..649e132b8 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -1220,6 +1220,18 @@ static struct symbol *evaluate_conditional_expression(struct expression *expr)
 
 Err:
 	expression_error(expr, "incompatible types in conditional expression (%s)", typediff);
+	/*
+	 * if the condition is constant, the type is in fact known
+	 * so use it, as gcc & clang do.
+	 */
+	switch (expr_truth_value(expr->conditional)) {
+	case 1:	expr->ctype = ltype;
+		break;
+	case 0: expr->ctype = rtype;
+		break;
+	default:
+		break;
+	}
 	return NULL;
 
 out:
diff --git a/validation/cond-err-expand.c b/validation/cond-err-expand.c
new file mode 100644
index 000000000..93bbac153
--- /dev/null
+++ b/validation/cond-err-expand.c
@@ -0,0 +1,27 @@
+static inline void f(void)
+{
+	__builtin_constant_p(0);
+}
+
+void foo(void)
+{
+	0 ? 0 : f();
+}
+
+void bar(void)
+{
+	1 ? f() : 0;
+}
+
+/*
+ * check-name: cond-err-expand.c
+ * check-command: test-linearize -Wno-decl $file
+ *
+ * check-error-start
+cond-err-expand.c:8:11: error: incompatible types in conditional expression (different base types)
+cond-err-expand.c:13:11: error: incompatible types in conditional expression (different base types)
+ * check-error-end
+ *
+ * check-output-ignore
+ * check-excludes: call.* __builtin_constant_p
+ */
-- 
2.13.2

Re: [PATCH v2 2/2] fix: give a type to bad cond expr with known condition

[Christopher Li]

On Fri, Aug 4, 2017 at 12:06 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> Conditional expressions whose second & third operands
> have non-compatible types are not conform to the C standard
> and sparse emit a warning for them and return the expression
> as being erroneous. In consequence, such expressions are not
> given a type. This, in turn, makes that some further processing
> cannot be done (correctly).
>
> It seems that neither GCC nor clang emit a warning when
> there is a type mismatch but the condition is a constant.
>
> In the case we're interested here (the slow compilation of a file)
> the operation that cannot be done is the expansion its operands.
> This, in turn and among other things, makes that builtins like
> __builtin_constant_p() are not evaluated with disatrous consequence
> for the amount of work done in the next phases.
>
> Fix this by giving to conditional expressions with constant
> condition the same type as the operand selected by the conditional
> (but keeping the warning) as GCC & clang seems to do.

Looks good to me.

Chris