Re: [Patch] Fix PR44328 - switch/case optimization produces an invalid lookup table index

Re: [Patch] Fix PR44328 - switch/case optimization produces an invalid lookup table index

[H.J. Lu]

On Thu, Aug 12, 2010 at 2:32 AM, Ian Bolton <Ian.Bolton@arm.com> wrote:
> This patch fixes PR44328, and provides a basic regression test for it.
>
> The bug was caused by an underflow occurring in tree-switch-conversion.c,
> when the lower bound for the valid switch range was subtracted from the
> minimum value for the enum, thereby causing values beyond the enum's max
> value to be possible.  Without this change, tree-vrp.c was optimising away
> the range check because it seemed unnecessary based on the max value it
> had for the index.
>
> Note that the bug does not show on trunk unless you use -fstrict-enums.
> My testing has been done on 4.4 branch and 4.5 branch for ARM.
>
> I would appreciate some feedback on how to make the test better (e.g.
> where to put it, should it be executed too?), and advice on whether
> the patch should be aimed at 4.5 or trunk initially.
>
> Cheers,
> Ian
>
>
> 2010-08-12  Ian Bolton  <ian.bolton@arm.com>
>
>        * tree-switch-conversion.c (gen_inbound_check): Ensure that the
>        type for the conditional has wide enough range.
>
>        * testsuite/gcc.target/arm/pr44328.C: New test.
>

Why is this test ARM specific?

-- 
H.J.

RE: [Patch] Fix PR44328 - switch/case optimization produces an invalid lookup table index

[Ian Bolton]

H.J.Lu writes:
> >        * testsuite/gcc.target/arm/pr44328.C: New test.
> >
> 
> Why is this test ARM specific?

It's because I didn't know the right place to put it yet. ;-)

I am open to suggestions as to where it should go, and what
options to give it.

For trunk, I need "-fstrict-enums -O2 -Wextra" to cause the
circumstances that exposed the bug, and check that there are
no warnings.  For 4.5 branch, -fstrict-enums does not exist,
so I guess I only want "-O2 -Wextra".

Cheers,
Ian

Re: [Patch] Fix PR44328 - switch/case optimization produces an invalid lookup table index

[H.J. Lu]

On Thu, Aug 12, 2010 at 9:28 AM, Ian Bolton <Ian.Bolton@arm.com> wrote:
> H.J.Lu writes:
>> >        * testsuite/gcc.target/arm/pr44328.C: New test.
>> >
>>
>> Why is this test ARM specific?
>
> It's because I didn't know the right place to put it yet. ;-)
>
> I am open to suggestions as to where it should go, and what
> options to give it.

gcc.dg?

> For trunk, I need "-fstrict-enums -O2 -Wextra" to cause the
> circumstances that exposed the bug, and check that there are
> no warnings.  For 4.5 branch, -fstrict-enums does not exist,
> so I guess I only want "-O2 -Wextra".
>

Only add -fstrict-enums to test in trunk.


-- 
H.J.

RE: [Patch] Fix PR44328 - switch/case optimization produces an invalid lookup table index

[Ian Bolton]

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

H.J.Lu writes:
> I am open to suggestions as to where it should go, and what
> > options to give it.
> 
> gcc.dg?
> 
> > For trunk, I need "-fstrict-enums -O2 -Wextra" to cause the
> > circumstances that exposed the bug, and check that there are
> > no warnings.  For 4.5 branch, -fstrict-enums does not exist,
> > so I guess I only want "-O2 -Wextra".
> >
> 
> Only add -fstrict-enums to test in trunk.
> 

Thanks for your help.  As the test is a C++ one, I have added it
to testsuite/g++.dg, and updated the patch accordingly.

Note that this is intended for 4.5 branch, as I have not
supplied -fstrict-enums in the test.

Best regards,
Ian



2010-08-16  Ian Bolton  <ian.bolton@arm.com>

	* tree-switch-conversion.c (gen_inbound_check): Ensure that the
	type for the conditional has wide enough range.

	* testsuite/g++.dg/pr44328.C: New test.

[-- Attachment #2: pr44328.patch --]
[-- Type: application/octet-stream, Size: 2075 bytes --]

Index: gcc/tree-switch-conversion.c
===================================================================
--- gcc/tree-switch-conversion.c	(revision 162573)
+++ gcc/tree-switch-conversion.c	(working copy)
@@ -96,6 +96,7 @@ eight) times the number of the actual sw
 #include "diagnostic.h"
 #include "tree-dump.h"
 #include "timevar.h"
+#include "langhooks.h"
 
 /* The main structure of the pass.  */
 struct switch_conv_info
@@ -693,9 +694,11 @@ gen_inbound_check (gimple swtch)
 
   /* Make sure we do not generate arithmetics in a subrange.  */
   if (TREE_TYPE (TREE_TYPE (info.index_expr)))
-    utype = unsigned_type_for (TREE_TYPE (TREE_TYPE (info.index_expr)));
+    utype = lang_hooks.types.type_for_mode
+      ( TYPE_MODE (TREE_TYPE (TREE_TYPE (info.index_expr))), 1);
   else
-    utype = unsigned_type_for (TREE_TYPE (info.index_expr));
+    utype = lang_hooks.types.type_for_mode
+      ( TYPE_MODE (TREE_TYPE (info.index_expr)), 1);
 
   /* (end of) block 0 */
   gsi = gsi_for_stmt (info.arr_ref_first);
--- /dev/null	2010-05-18 17:29:48.296470258 +0100
+++ gcc/testsuite/g++.dg/pr44328.C	2010-08-16 15:01:02.821881621 +0100
@@ -0,0 +1,40 @@
+/* PR44328 */
+/* { dg-do compile } */
+/* { dg-options "-c -O2 -Wextra" } */
+#define O_RDONLY     (1<<0)
+#define O_WRONLY     (1<<1)
+#define O_RDWR       (O_RDONLY|O_WRONLY)
+#define O_CREAT      (1<<3)
+#define O_TRUNC      (1<<6)
+
+typedef enum {
+    OM_READ = 0,
+    OM_WRITE,
+    OM_READWRITE_NOCREATE,
+    OM_READWRITE_CREATE
+} OpenMode;
+
+extern int open(const char *name, int mode);
+
+void open_file(const char *filename, const OpenMode rw)
+{
+    int mode = 0;
+
+    switch( rw )
+    {
+    case OM_WRITE:
+        mode = O_WRONLY|O_CREAT|O_TRUNC;
+        break;
+    case OM_READ:
+        mode = O_RDONLY;
+        break;
+    case OM_READWRITE_NOCREATE:
+        mode = O_RDWR;
+        break;
+    case OM_READWRITE_CREATE:
+        mode = O_RDWR|O_CREAT|O_TRUNC;
+        break;
+    }
+
+    open( filename, mode );
+}

Re: [Patch] Fix PR44328 - switch/case optimization produces an invalid lookup table index

[H.J. Lu]

On Mon, Aug 16, 2010 at 7:35 AM, Ian Bolton <Ian.Bolton@arm.com> wrote:
> H.J.Lu writes:
>> I am open to suggestions as to where it should go, and what
>> > options to give it.
>>
>> gcc.dg?
>>
>> > For trunk, I need "-fstrict-enums -O2 -Wextra" to cause the
>> > circumstances that exposed the bug, and check that there are
>> > no warnings.  For 4.5 branch, -fstrict-enums does not exist,
>> > so I guess I only want "-O2 -Wextra".
>> >
>>
>> Only add -fstrict-enums to test in trunk.
>>
>
> Thanks for your help.  As the test is a C++ one, I have added it
> to testsuite/g++.dg, and updated the patch accordingly.
>
> Note that this is intended for 4.5 branch, as I have not
> supplied -fstrict-enums in the test.
>
> Best regards,
> Ian
>
>
>
> 2010-08-16  Ian Bolton  <ian.bolton@arm.com>
>
>        * tree-switch-conversion.c (gen_inbound_check): Ensure that the
>        type for the conditional has wide enough range.
>
>        * testsuite/g++.dg/pr44328.C: New test.
>

You should submit the patch for trunk first and backport to 4.5 branch
after it was checked into trunk.

-- 
H.J.

RE: [Patch] Fix PR44328 - switch/case optimization produces an invalid lookup table index

[Ian Bolton]

H.J.Lu writes:
> > H.J.Lu writes:
> >> I am open to suggestions as to where it should go, and what
> >> > options to give it.
> >>
> >> gcc.dg?
> >>
> >> > For trunk, I need "-fstrict-enums -O2 -Wextra" to cause the
> >> > circumstances that exposed the bug, and check that there are
> >> > no warnings.  For 4.5 branch, -fstrict-enums does not exist,
> >> > so I guess I only want "-O2 -Wextra".
> >> >
> >>
> >> Only add -fstrict-enums to test in trunk.
> >>
> >
> > Thanks for your help.  As the test is a C++ one, I have added it
> > to testsuite/g++.dg, and updated the patch accordingly.
> >
> > Note that this is intended for 4.5 branch, as I have not
> > supplied -fstrict-enums in the test.
> >
> > Best regards,
> > Ian
> >
> >
> >
> > 2010-08-16  Ian Bolton  <ian.bolton@arm.com>
> >
> >        * tree-switch-conversion.c (gen_inbound_check): Ensure that
> the
> >        type for the conditional has wide enough range.
> >
> >        * testsuite/g++.dg/pr44328.C: New test.
> >
> 
> You should submit the patch for trunk first and backport to 4.5 branch
> after it was checked into trunk.

I have ran into trouble creating a test-case for trunk, because the
second invocation of tree_forwprop hides the issue.  I have hacked the
code to only allow the first pass of tree_forwprop and it can then be
seen that vrp1 erroneously turns <= 2 into != 3 due to having the wrong
type for the switch index.  (My fix prevents this from happening.)

What do you suggest I do with regards the patch for trunk?  Should I
include no test, or should I include the test I created for 4.5 branch,
even though it won't actually catch the trunk issue?

Best regards,
Ian

Re: [Patch] Fix PR44328 - switch/case optimization produces an invalid lookup table index

[Richard Guenther]

On Tue, Aug 17, 2010 at 6:06 PM, Ian Bolton <Ian.Bolton@arm.com> wrote:
> H.J.Lu writes:
>> > H.J.Lu writes:
>> >> I am open to suggestions as to where it should go, and what
>> >> > options to give it.
>> >>
>> >> gcc.dg?
>> >>
>> >> > For trunk, I need "-fstrict-enums -O2 -Wextra" to cause the
>> >> > circumstances that exposed the bug, and check that there are
>> >> > no warnings.  For 4.5 branch, -fstrict-enums does not exist,
>> >> > so I guess I only want "-O2 -Wextra".
>> >> >
>> >>
>> >> Only add -fstrict-enums to test in trunk.
>> >>
>> >
>> > Thanks for your help.  As the test is a C++ one, I have added it
>> > to testsuite/g++.dg, and updated the patch accordingly.
>> >
>> > Note that this is intended for 4.5 branch, as I have not
>> > supplied -fstrict-enums in the test.
>> >
>> > Best regards,
>> > Ian
>> >
>> >
>> >
>> > 2010-08-16  Ian Bolton  <ian.bolton@arm.com>
>> >
>> >        * tree-switch-conversion.c (gen_inbound_check): Ensure that
>> the
>> >        type for the conditional has wide enough range.
>> >
>> >        * testsuite/g++.dg/pr44328.C: New test.
>> >
>>
>> You should submit the patch for trunk first and backport to 4.5 branch
>> after it was checked into trunk.
>
> I have ran into trouble creating a test-case for trunk, because the
> second invocation of tree_forwprop hides the issue.  I have hacked the
> code to only allow the first pass of tree_forwprop and it can then be
> seen that vrp1 erroneously turns <= 2 into != 3 due to having the wrong
> type for the switch index.  (My fix prevents this from happening.)
>
> What do you suggest I do with regards the patch for trunk?  Should I
> include no test, or should I include the test I created for 4.5 branch,
> even though it won't actually catch the trunk issue?

Do the latter - use the 4.5 testcase.

Richard.

> Best regards,
> Ian
>
>
>

RE: [Patch] Fix PR44328 - switch/case optimization produces an invalid lookup table index

[Ian Bolton]

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

> > What do you suggest I do with regards the patch for trunk?  Should I
> > include no test, or should I include the test I created for 4.5
> branch,
> > even though it won't actually catch the trunk issue?
> 
> Do the latter - use the 4.5 testcase.
> 

In that case, here is my patch for trunk.

I have run regression suite for ARM and bootstrapped x86 also.

OK to commit?

Cheers,
Ian


2010-08-18  Ian Bolton  <ian.bolton@arm.com>

	* tree-switch-conversion.c (gen_inbound_check): Ensure that the
	type for the conditional has wide enough range.

	* testsuite/g++.dg/pr44328.C: New test.

[-- Attachment #2: pr44328.trunk.patch --]
[-- Type: application/octet-stream, Size: 2055 bytes --]

diff --git a/gcc/tree-switch-conversion.c b/gcc/tree-switch-conversion.c
index ed8b5ce..a5ab7a1 100644
--- a/gcc/tree-switch-conversion.c
+++ b/gcc/tree-switch-conversion.c
@@ -96,6 +96,7 @@ eight) times the number of the actual switch branches. */
 #include "gimple-pretty-print.h"
 #include "tree-dump.h"
 #include "timevar.h"
+#include "langhooks.h"
 
 /* The main structure of the pass.  */
 struct switch_conv_info
@@ -694,9 +695,11 @@ gen_inbound_check (gimple swtch)
 
   /* Make sure we do not generate arithmetics in a subrange.  */
   if (TREE_TYPE (TREE_TYPE (info.index_expr)))
-    utype = unsigned_type_for (TREE_TYPE (TREE_TYPE (info.index_expr)));
+    utype = lang_hooks.types.type_for_mode
+      ( TYPE_MODE (TREE_TYPE (TREE_TYPE (info.index_expr))), 1);
   else
-    utype = unsigned_type_for (TREE_TYPE (info.index_expr));
+    utype = lang_hooks.types.type_for_mode
+      ( TYPE_MODE (TREE_TYPE (info.index_expr)), 1);
 
   /* (end of) block 0 */
   gsi = gsi_for_stmt (info.arr_ref_first);
--- /dev/null	2010-08-18 10:27:48.304302849 +0100
+++ gcc/testsuite/g++.dg/pr44328.C	2010-08-18 11:21:50.406102710 +0100
@@ -0,0 +1,39 @@
+/* { dg-do compile } */
+/* { dg-options "-c -O2 -Wextra" } */
+#define O_RDONLY     (1<<0)
+#define O_WRONLY     (1<<1)
+#define O_RDWR       (O_RDONLY|O_WRONLY)
+#define O_CREAT      (1<<3)
+#define O_TRUNC      (1<<6)
+
+typedef enum {
+    OM_READ = 0,
+    OM_WRITE,
+    OM_READWRITE_NOCREATE,
+    OM_READWRITE_CREATE
+} OpenMode;
+
+extern int open(const char *name, int mode);
+
+void open_file(const char *filename, const OpenMode rw)
+{
+    int mode = 0;
+
+    switch( rw )
+    {
+    case OM_WRITE:
+        mode = O_WRONLY|O_CREAT|O_TRUNC;
+        break;
+    case OM_READ:
+        mode = O_RDONLY;
+        break;
+    case OM_READWRITE_NOCREATE:
+        mode = O_RDWR;
+        break;
+    case OM_READWRITE_CREATE:
+        mode = O_RDWR|O_CREAT|O_TRUNC;
+        break;
+    }
+
+    open( filename, mode );
+}

Re: [Patch] Fix PR44328 - switch/case optimization produces an invalid lookup table index

[Richard Guenther]

On Wed, Aug 18, 2010 at 12:34 PM, Ian Bolton <Ian.Bolton@arm.com> wrote:
>> > What do you suggest I do with regards the patch for trunk?  Should I
>> > include no test, or should I include the test I created for 4.5
>> branch,
>> > even though it won't actually catch the trunk issue?
>>
>> Do the latter - use the 4.5 testcase.
>>
>
> In that case, here is my patch for trunk.
>
> I have run regression suite for ARM and bootstrapped x86 also.
>
> OK to commit?

Ok.

Thanks,
Richard.

> Cheers,
> Ian
>
>
> 2010-08-18  Ian Bolton  <ian.bolton@arm.com>
>
>        * tree-switch-conversion.c (gen_inbound_check): Ensure that the
>        type for the conditional has wide enough range.
>
>        * testsuite/g++.dg/pr44328.C: New test.
>

Re: [Patch] Fix PR44328 - switch/case optimization produces an invalid lookup table index

[Jakub Jelinek]

On Wed, Aug 18, 2010 at 11:34:49AM +0100, Ian Bolton wrote:
> > > What do you suggest I do with regards the patch for trunk?  Should I
> > > include no test, or should I include the test I created for 4.5
> > branch,
> > > even though it won't actually catch the trunk issue?
> > 
> > Do the latter - use the 4.5 testcase.
> > 
> 
> In that case, here is my patch for trunk.
> 
> I have run regression suite for ARM and bootstrapped x86 also.

Tha formatting is wrong, no space should be after ( .

	Jakub

RE: [Patch] Fix PR44328 - switch/case optimization produces an invalid lookup table index

[Ian Bolton]

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

> Tha formatting is wrong, no space should be after ( .

Well spotted.  Here's the amended version.

All OK now?

Cheers,
Ian


2010-08-18  Ian Bolton  <ian.bolton@arm.com>

	* tree-switch-conversion.c (gen_inbound_check): Ensure that the
	type for the conditional has wide enough range.

	* testsuite/g++.dg/pr44328.C: New test.

[-- Attachment #2: pr44328.trunk.patch --]
[-- Type: application/octet-stream, Size: 2053 bytes --]

diff --git a/gcc/tree-switch-conversion.c b/gcc/tree-switch-conversion.c
index ed8b5ce..a5ab7a1 100644
--- a/gcc/tree-switch-conversion.c
+++ b/gcc/tree-switch-conversion.c
@@ -96,6 +96,7 @@ eight) times the number of the actual switch branches. */
 #include "gimple-pretty-print.h"
 #include "tree-dump.h"
 #include "timevar.h"
+#include "langhooks.h"
 
 /* The main structure of the pass.  */
 struct switch_conv_info
@@ -694,9 +695,11 @@ gen_inbound_check (gimple swtch)
 
   /* Make sure we do not generate arithmetics in a subrange.  */
   if (TREE_TYPE (TREE_TYPE (info.index_expr)))
-    utype = unsigned_type_for (TREE_TYPE (TREE_TYPE (info.index_expr)));
+    utype = lang_hooks.types.type_for_mode
+      (TYPE_MODE (TREE_TYPE (TREE_TYPE (info.index_expr))), 1);
   else
-    utype = unsigned_type_for (TREE_TYPE (info.index_expr));
+    utype = lang_hooks.types.type_for_mode
+      (TYPE_MODE (TREE_TYPE (info.index_expr)), 1);
 
   /* (end of) block 0 */
   gsi = gsi_for_stmt (info.arr_ref_first);
--- /dev/null	2010-08-18 10:27:48.304302849 +0100
+++ gcc/testsuite/g++.dg/pr44328.C	2010-08-18 11:21:50.406102710 +0100
@@ -0,0 +1,39 @@
+/* { dg-do compile } */
+/* { dg-options "-c -O2 -Wextra" } */
+#define O_RDONLY     (1<<0)
+#define O_WRONLY     (1<<1)
+#define O_RDWR       (O_RDONLY|O_WRONLY)
+#define O_CREAT      (1<<3)
+#define O_TRUNC      (1<<6)
+
+typedef enum {
+    OM_READ = 0,
+    OM_WRITE,
+    OM_READWRITE_NOCREATE,
+    OM_READWRITE_CREATE
+} OpenMode;
+
+extern int open(const char *name, int mode);
+
+void open_file(const char *filename, const OpenMode rw)
+{
+    int mode = 0;
+
+    switch( rw )
+    {
+    case OM_WRITE:
+        mode = O_WRONLY|O_CREAT|O_TRUNC;
+        break;
+    case OM_READ:
+        mode = O_RDONLY;
+        break;
+    case OM_READWRITE_NOCREATE:
+        mode = O_RDWR;
+        break;
+    case OM_READWRITE_CREATE:
+        mode = O_RDWR|O_CREAT|O_TRUNC;
+        break;
+    }
+
+    open( filename, mode );
+}

Re: [Patch] Fix PR44328 - switch/case optimization produces an invalid lookup table index

[Martin Jambor]

Hi,

On Wed, Aug 18, 2010 at 11:51:44AM +0100, Ian Bolton wrote:
> > Tha formatting is wrong, no space should be after ( .
> 
> Well spotted.  Here's the amended version.
> 

Thanks for looking into this problem.  However, I believe there is one
omission...

> All OK now?
> 
> Cheers,
> Ian
> 
> 
> 2010-08-18  Ian Bolton  <ian.bolton@arm.com>
> 
> 	* tree-switch-conversion.c (gen_inbound_check): Ensure that the
> 	type for the conditional has wide enough range.
> 
> 	* testsuite/g++.dg/pr44328.C: New test.
>
> diff --git a/gcc/tree-switch-conversion.c b/gcc/tree-switch-conversion.c
> index ed8b5ce..a5ab7a1 100644
> --- a/gcc/tree-switch-conversion.c       
> +++ b/gcc/tree-switch-conversion.c
> @@ -96,6 +96,7 @@ eight) times the number of the actual switch branches. */
>  #include "gimple-pretty-print.h"
>  #include "tree-dump.h"
>  #include "timevar.h"
> +#include "langhooks.h"
> 

This change is not reflected in Makefile.in.  Can you please add
langhooks.h to the dependencies of tree-switch-conversion.o?  (I'm
still in the middle of emerging from post-vacation email quagmire.)

Thanks,

Martin

>  /* The main structure of the pass.  */
>  struct switch_conv_info

RE: [Patch] Fix PR44328 - switch/case optimization produces an invalid lookup table index

[Ian Bolton]

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

Martin Jambor writes:
> > diff --git a/gcc/tree-switch-conversion.c b/gcc/tree-switch-
> conversion.c
> > index ed8b5ce..a5ab7a1 100644
> > --- a/gcc/tree-switch-conversion.c
> > +++ b/gcc/tree-switch-conversion.c
> > @@ -96,6 +96,7 @@ eight) times the number of the actual switch
> branches. */
> >  #include "gimple-pretty-print.h"
> >  #include "tree-dump.h"
> >  #include "timevar.h"
> > +#include "langhooks.h"
> >
> 
> This change is not reflected in Makefile.in.  Can you please add
> langhooks.h to the dependencies of tree-switch-conversion.o?  (I'm
> still in the middle of emerging from post-vacation email quagmire.)

I've updated gcc/Makefile.in accordingly.  (See attached).

OK to commit?

Ian


2010-09-01  Ian Bolton  <ian.bolton@arm.com>

        * Makefile.in (tree-switch-conversion.o): Update dependencies.

[-- Attachment #2: Makefile.in.diff --]
[-- Type: application/octet-stream, Size: 714 bytes --]

Index: gcc/Makefile.in
===================================================================
--- gcc/Makefile.in	(revision 163732)
+++ gcc/Makefile.in	(working copy)
@@ -3147,7 +3147,7 @@ tree-switch-conversion.o : tree-switch-c
     $(TIMEVAR_H) $(TM_H) coretypes.h $(TREE_DUMP_H) $(GIMPLE_H) \
     $(TREE_PASS_H) $(FLAGS_H) $(EXPR_H) $(BASIC_BLOCK_H) output.h \
     $(GGC_H) $(OBSTACK_H) $(PARAMS_H) $(CPPLIB_H) $(PARAMS_H) \
-    gimple-pretty-print.h
+    gimple-pretty-print.h langhooks.h
 tree-complex.o : tree-complex.c $(CONFIG_H) $(SYSTEM_H) coretypes.h $(TREE_H) \
     $(TM_H) $(FLAGS_H) $(TREE_FLOW_H) $(GIMPLE_H) \
     tree-iterator.h $(TREE_PASS_H) tree-ssa-propagate.h $(DIAGNOSTIC_H)

Re: [Patch] Fix PR44328 - switch/case optimization produces an invalid lookup table index

[Richard Guenther]

On Wed, Sep 1, 2010 at 4:25 PM, Ian Bolton <ian.bolton@arm.com> wrote:
> Martin Jambor writes:
>> > diff --git a/gcc/tree-switch-conversion.c b/gcc/tree-switch-
>> conversion.c
>> > index ed8b5ce..a5ab7a1 100644
>> > --- a/gcc/tree-switch-conversion.c
>> > +++ b/gcc/tree-switch-conversion.c
>> > @@ -96,6 +96,7 @@ eight) times the number of the actual switch
>> branches. */
>> >  #include "gimple-pretty-print.h"
>> >  #include "tree-dump.h"
>> >  #include "timevar.h"
>> > +#include "langhooks.h"
>> >
>>
>> This change is not reflected in Makefile.in.  Can you please add
>> langhooks.h to the dependencies of tree-switch-conversion.o?  (I'm
>> still in the middle of emerging from post-vacation email quagmire.)
>
> I've updated gcc/Makefile.in accordingly.  (See attached).
>
> OK to commit?

Ok.

Thanks,
Richard.

> Ian
>
>
> 2010-09-01  Ian Bolton  <ian.bolton@arm.com>
>
>        * Makefile.in (tree-switch-conversion.o): Update dependencies.
>

Re: [Patch] Fix PR44328 - switch/case optimization produces an invalid lookup table index

[Diego Novillo]

On Wed, Sep 1, 2010 at 10:25, Ian Bolton <ian.bolton@arm.com> wrote:

> 2010-09-01  Ian Bolton  <ian.bolton@arm.com>
>
>        * Makefile.in (tree-switch-conversion.o): Update dependencies.

OK (qualifies as obvious).


Diego.

Re: [Patch] Fix PR44328 - switch/case optimization produces an invalid lookup table index

[Martin Jambor]

Hi,

On Wed, Sep 01, 2010 at 03:25:59PM +0100, Ian Bolton wrote:
> Martin Jambor writes:
> > > diff --git a/gcc/tree-switch-conversion.c b/gcc/tree-switch-
> > conversion.c
> > > index ed8b5ce..a5ab7a1 100644
> > > --- a/gcc/tree-switch-conversion.c
> > > +++ b/gcc/tree-switch-conversion.c
> > > @@ -96,6 +96,7 @@ eight) times the number of the actual switch
> > branches. */
> > >  #include "gimple-pretty-print.h"
> > >  #include "tree-dump.h"
> > >  #include "timevar.h"
> > > +#include "langhooks.h"
> > >
> > 
> > This change is not reflected in Makefile.in.  Can you please add
> > langhooks.h to the dependencies of tree-switch-conversion.o?  (I'm
> > still in the middle of emerging from post-vacation email quagmire.)
> 
> I've updated gcc/Makefile.in accordingly.  (See attached).
> 
> OK to commit?
> 
> Ian
> 
> 
> 2010-09-01  Ian Bolton  <ian.bolton@arm.com>
> 
>         * Makefile.in (tree-switch-conversion.o): Update dependencies.


I don't have the right to approve anything but I'd say this is
"obvious."  (You should still verify that gcc compiles - or in theory
that it also bootstraps and has no testsuite regressions - with the
change just in case we both overlooked something).

By the way, unless you have a good reason to do otherwise, could you
please post your patches inline in the email message rather than
attaching them?  It makes life a bit easier for readers (and
especially for those that reply with comments).

Above all, thanks a lot for addressing this,

Martin

[Patch] Fix PR44328 - switch/case optimization produces an invalid lookup table index

[Ian Bolton]

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

This patch fixes PR44328, and provides a basic regression test for it.

The bug was caused by an underflow occurring in tree-switch-conversion.c,
when the lower bound for the valid switch range was subtracted from the
minimum value for the enum, thereby causing values beyond the enum's max
value to be possible.  Without this change, tree-vrp.c was optimising away
the range check because it seemed unnecessary based on the max value it
had for the index.

Note that the bug does not show on trunk unless you use -fstrict-enums.
My testing has been done on 4.4 branch and 4.5 branch for ARM.

I would appreciate some feedback on how to make the test better (e.g.
where to put it, should it be executed too?), and advice on whether
the patch should be aimed at 4.5 or trunk initially.

Cheers,
Ian


2010-08-12  Ian Bolton  <ian.bolton@arm.com>

	* tree-switch-conversion.c (gen_inbound_check): Ensure that the
	type for the conditional has wide enough range.

	* testsuite/gcc.target/arm/pr44328.C: New test.

[-- Attachment #2: pr44328.patch --]
[-- Type: application/octet-stream, Size: 2103 bytes --]

Index: gcc/tree-switch-conversion.c
===================================================================
--- gcc/tree-switch-conversion.c	(revision 162573)
+++ gcc/tree-switch-conversion.c	(working copy)
@@ -96,6 +96,7 @@ eight) times the number of the actual sw
 #include "diagnostic.h"
 #include "tree-dump.h"
 #include "timevar.h"
+#include "langhooks.h"
 
 /* The main structure of the pass.  */
 struct switch_conv_info
@@ -693,9 +694,11 @@ gen_inbound_check (gimple swtch)
 
   /* Make sure we do not generate arithmetics in a subrange.  */
   if (TREE_TYPE (TREE_TYPE (info.index_expr)))
-    utype = unsigned_type_for (TREE_TYPE (TREE_TYPE (info.index_expr)));
+    utype = lang_hooks.types.type_for_mode
+      ( TYPE_MODE (TREE_TYPE (TREE_TYPE (info.index_expr))), 1);
   else
-    utype = unsigned_type_for (TREE_TYPE (info.index_expr));
+    utype = lang_hooks.types.type_for_mode
+      ( TYPE_MODE (TREE_TYPE (info.index_expr)), 1);
 
   /* (end of) block 0 */
   gsi = gsi_for_stmt (info.arr_ref_first);
--- /dev/null	2010-05-18 17:29:48.296470258 +0100
+++ gcc/testsuite/gcc.target/arm/pr44328.C	2010-08-11 15:49:00.434300055 +0100
@@ -0,0 +1,40 @@
+/* PR44328 */
+/* { dg-do compile { target arm*-*-* } } */
+/* { dg-options "-c -O2 -Wextra" } */
+#define O_RDONLY     (1<<0)
+#define O_WRONLY     (1<<1)
+#define O_RDWR       (O_RDONLY|O_WRONLY)
+#define O_CREAT      (1<<3)
+#define O_TRUNC      (1<<6)
+
+typedef enum {
+    OM_READ = 0,
+    OM_WRITE,
+    OM_READWRITE_NOCREATE,
+    OM_READWRITE_CREATE
+} OpenMode;
+
+extern int open(const char *name, int mode);
+
+void open_file(const char *filename, const OpenMode rw)
+{
+    int mode = 0;
+
+    switch( rw )
+    {
+    case OM_WRITE:
+        mode = O_WRONLY|O_CREAT|O_TRUNC;
+        break;
+    case OM_READ:
+        mode = O_RDONLY;
+        break;
+    case OM_READWRITE_NOCREATE:
+        mode = O_RDWR;
+        break;
+    case OM_READWRITE_CREATE:
+        mode = O_RDWR|O_CREAT|O_TRUNC;
+        break;
+    }
+
+    open( filename, mode );
+}