diff mbox

linearize bug?

Message ID 201108271854.40694.kdudka@redhat.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Kamil Dudka Aug. 27, 2011, 4:54 p.m. UTC
On Saturday 27 August 2011 17:53:23 Linus Torvalds wrote:
> The attached patch is ENTIRELY UNTESTED. The only thing I tested it on
> is your test-case. Running "make test" requires stuff that I don't
> even have installed, and I'm lazy ;^o

It seems to break loops with no conditions.  The following piece of code:

#include <stdio.h>

int main()
{
    for(;;)
        printf("");
}

... translates into:

main:
.L0x7f233ed67010:
        <entry-point>
        call.32     %r2 <- printf, ""
        ret.32

... which is obviously wrong.  I am not sure if handling that special case 
separately (as done in the attached patch) is a correct way to fix it.

Kamil

Comments

Linus Torvalds Aug. 27, 2011, 5:27 p.m. UTC | #1
On Sat, Aug 27, 2011 at 10:13 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> But your patch is fine too.

Hmm. There's badness going on with that patch: I can't check the
kernel source tree, I get a

  CHECK   arch/x86/kernel/process_64.c
  sparse: simplify.c:82: if_convert_phi: Assertion `br->cond' failed.

so clearly something is screwed up. It happens with your version too,
it seems to have gotten triggered by the loop rewriting change.

Which *should* have been semantically no difference what-so-ever. But
it's been so long, I can't recall all the rules. It may be that trying
to linearize the same expression twice (once for the entry condition,
once for the loop end) was just something invalid.

So the patch is just broken. Never mind.

                        Linus
--
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
Linus Torvalds Aug. 27, 2011, 7:26 p.m. UTC | #2
On Sat, Aug 27, 2011 at 10:27 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Which *should* have been semantically no difference what-so-ever. But
> it's been so long, I can't recall all the rules. It may be that trying
> to linearize the same expression twice (once for the entry condition,
> once for the loop end) was just something invalid.
>
> So the patch is just broken. Never mind.

Yes indeed.

The reason we cannot linearize the same expression twice is that we
associate some state with the expression. For example, label symbols
in statement expressions are associated with the basic block they get
linearized to. If you linearize the same expression twice, that kind
of thing will completely screw things up..

So throw away my patch to linearize.c. It was fundamentally broken. I
suspect we don't want to duplicate the conditional for anything but
simple expressions anyway, so if I get a big spurt of energy, I might
add some logic to say "if it's a really simple conditional, then
duplicate it", since then we won't have issues with statement
expressions etc anyway.

                    Linus
--
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

 linearize.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/linearize.c b/linearize.c
index 30e0c9d..9e4b6a4 100644
--- a/linearize.c
+++ b/linearize.c
@@ -2055,7 +2055,7 @@  pseudo_t linearize_statement(struct entrypoint *ep, struct statement *stmt)
 		struct statement  *statement = stmt->iterator_statement;
 		struct statement  *post_statement = stmt->iterator_post_statement;
 		struct expression *post_condition = stmt->iterator_post_condition;
-		struct basic_block *loop_top, *loop_body, *loop_continue, *loop_end;
+		struct basic_block *loop_body, *loop_continue, *loop_end;
 
 		concat_symbol_list(stmt->iterator_syms, &ep->syms);
 		linearize_statement(ep, pre_statement);
@@ -2078,9 +2078,12 @@  pseudo_t linearize_statement(struct entrypoint *ep, struct statement *stmt)
 		linearize_statement(ep, post_statement);
 
 		/* No post-condition means that it's the same as the pre-condition */
-		if (!post_condition)
-			linearize_cond_branch(ep, pre_condition, loop_body, loop_end);
-		else
+		if (!post_condition) {
+			if (pre_condition)
+				linearize_cond_branch(ep, pre_condition, loop_body, loop_end);
+			else
+				add_goto(ep, loop_body);
+		} else
 			linearize_cond_branch(ep, post_condition, loop_body, loop_end);
 		set_activeblock(ep, loop_end);
 		break;