missing warnings with -Warray-bounds

[Martin Uecker]

I am trying to get more useful warnings for array bounds.
In particular I am interested in cases like this:

void foo(int (*a)[3], int n, int (*b)[n])
{
(*a)[4] = 1;
(*b)[n + 1] = 1;
}

That currently even the first assignment does not produce a warning
is a bit disappointing. (clang does)

There is also no warning in the following example
when the array is the last element of a struct.

struct h3 {
int i;
int j[3];
};

struct h3* h3 = malloc(sizeof(struct h) + 3 * sizeof(int));
h3->j[4] = 1;

I guess this is to avoid warnings for the 'struct hack', but why 
is this not limited to arrays with size 0 (and maybe 1) and 
flexible array members?

The relevant code is in the function check_array_ref in 
gcc/tree-vrp.c (see below). It seems that this code accidentally 
deactivates all warnings for arrays accessed through a pointer. 
Or is there a reason for returning when this is not a struct member?

It also gives arrays which are the last member of a struct a free
pass - even if they have a size greater than 1.

Martin



 
  /* Accesses to trailing arrays via pointers may access storage
 beyond the types array bounds.  */
  base = get_base_address (ref);
  if (base && TREE_CODE (base) == MEM_REF)
{
  tree cref, next = NULL_TREE;

  if (TREE_CODE (TREE_OPERAND (ref, 0)) != COMPONENT_REF)
return;

  cref = TREE_OPERAND (ref, 0);
  if (TREE_CODE (TREE_TYPE (TREE_OPERAND (cref, 0))) == RECORD_TYPE)
for (next = DECL_CHAIN (TREE_OPERAND (cref, 1));
 next && TREE_CODE (next) != FIELD_DECL;
 next = DECL_CHAIN (next))
  ;

  /* If this is the last field in a struct type or a field in a
 union type do not warn.  */
  if (!next)
return;
}

Re: missing warnings with -Warray-bounds

[Jakub Jelinek]

On Mon, Nov 10, 2014 at 12:20:03AM -0800, Martin Uecker wrote:
> There is also no warning in the following example
> when the array is the last element of a struct.
> 
> struct h3 {
> int i;
> int j[3];
> };
> 
> struct h3* h3 = malloc(sizeof(struct h) + 3 * sizeof(int));
> h3->j[4] = 1;
> 
> I guess this is to avoid warnings for the 'struct hack', but why 
> is this not limited to arrays with size 0 (and maybe 1) and 
> flexible array members?

Because 0 or 1 are not the only ones recognized as poor man's flexible array
members, any trailing arrays are, whatever the constant is.  So it is very
much intentional we don't warn above.  You haven't provided struct h definition,
if you meant offsetof(struct h3, j[0]) or similar instead, then I think
-fsanitize=undefined should diagnose this at runtime (and of course
-fsanitize=address too).

Jakub

Re: missing warnings with -Warray-bounds

[Martin Uecker]

Jakub Jelinek :
> On Mon, Nov 10, 2014 at 12:20:03AM -0800, Martin Uecker wrote:
> > There is also no warning in the following example
> > when the array is the last element of a struct.
> > 
> > struct h3 {
> > int i;
> > int j[3];
> > };
> > 
> > struct h3* h3 = malloc(sizeof(struct h) + 3 * sizeof(int));
> > h3->j[4] = 1;
> > 
> > I guess this is to avoid warnings for the 'struct hack', but why 
> > is this not limited to arrays with size 0 (and maybe 1) and 
> > flexible array members?
> 
> Because 0 or 1 are not the only ones recognized as poor man's flexible array
> members, any trailing arrays are, whatever the constant is.  So it is very
> much intentional we don't warn above.  

Is such code common? Clang does warn in this case. 
The warning seems very useful to me and can easily be turned off. 
Or one could add -W(no-)warn-struct-hack if really needed.

Another odd case is:

struct h0b {
int i;
int j[0];
int k;
};

struct h0b* h0b = ...

h0b->j[4] = 1;  

> You haven't provided struct h definition,

Sorry, this should have been sizeof(struct h3).


Martin

Re: missing warnings with -Warray-bounds

[Jakub Jelinek]

On Mon, Nov 10, 2014 at 12:52:02AM -0800, Martin Uecker wrote:
> Jakub Jelinek :
> > On Mon, Nov 10, 2014 at 12:20:03AM -0800, Martin Uecker wrote:
> > > There is also no warning in the following example
> > > when the array is the last element of a struct.
> > > 
> > > struct h3 {
> > > int i;
> > > int j[3];
> > > };
> > > 
> > > struct h3* h3 = malloc(sizeof(struct h) + 3 * sizeof(int));
> > > h3->j[4] = 1;
> > > 
> > > I guess this is to avoid warnings for the 'struct hack', but why 
> > > is this not limited to arrays with size 0 (and maybe 1) and 
> > > flexible array members?
> > 
> > Because 0 or 1 are not the only ones recognized as poor man's flexible array
> > members, any trailing arrays are, whatever the constant is.  So it is very
> > much intentional we don't warn above.  
> 
> Is such code common?

Yes.

> Clang does warn in this case. 

Clang clearly doesn't care about false positives, it is driven by the desire
to emit as many warnings as possible.

> The warning seems very useful to me and can easily be turned off. 
> Or one could add -W(no-)warn-struct-hack if really needed.
> 
> Another odd case is:
> 
> struct h0b {
>   int i;
>   int j[0];
>   int k;
> };
> 
> struct h0b* h0b = ...
> 
> h0b->j[4] = 1;  

-fsanitize=undefined should catch this.

> > You haven't provided struct h definition,
> 
> Sorry, this should have been sizeof(struct h3).

In that case the code you've posted is valid, there should be no warnings or
runtime error messages.

Jakub

Re: missing warnings with -Warray-bounds

[Martin Uecker]

Jakub Jelinek :
> On Mon, Nov 10, 2014 at 12:52:02AM -0800, Martin Uecker wrote:
> > Jakub Jelinek :

...
> > The warning seems very useful to me and can easily be turned off. 
> > Or one could add -W(no-)warn-struct-hack if really needed.
> > 
> > Another odd case is:
> > 
> > struct h0b {
> > int i;
> > int j[0];
> > int k;
> > };
> > 
> > struct h0b* h0b = ...
> > 
> > h0b->j[4] = 1;  
> 
> -fsanitize=undefined should catch this.

sanitize is not a replacment for a compile-time warning. The later
are much more useful.
 
> > > You haven't provided struct h definition,
> > 
> > Sorry, this should have been sizeof(struct h3).
> 
> In that case the code you've posted is valid, there should be no warnings or
> runtime error messages.

I meant malloc(sizeof(h3)) without the additional term.

But it is undefined behavior in any case, so I would expect
a compiler to give a warning.

Martin

Re: testing policy for C/C++ front end changes

[Richard Biener]

On Mon, Nov 10, 2014 at 5:50 AM, Jeff Law  wrote:
> On 11/09/14 16:13, Sandra Loosemore wrote:
>>
>> https://gcc.gnu.org/contribute.html#testing
>>
>> and noticed that the policy is to require a complete bootstrap for C
>> changes, but not for C++.  Given that GCC's implementation language is
>> now C++, isn't that backwards?  I'm not trying to weasel out of the
>> extra work for my patch, just curious if the web site guidelines have
>> gotten bit-rotten after the switch to C++, or if the SC did indeed
>> consider the issue already and the published policy is accurate.
>
> They've bit-rotted a bit.  Interested in cons-ing up an update?

Bootstrap should now be required for both C and C++ FE changes
_and_ for libstdc++ changes as well given we start to pull in
libstdc++ headers during bootstrap.

Richard.

> jeff
>

arm/thumb broken on head

[Joel Sherrill]

Hi

With gcc, newlib, and binutils head, arm-rtems and arm-eabi both
die building libgcc2.c for the Thumb. I don't know if this is a recent
gcc change or binutils having added some new error checking. Anyone
got any ideas?

/users/joel/test-gcc/b-arm-eabi-gcc/./gcc/xgcc
-B/users/joel/test-gcc/b-arm-eabi-gcc/./gcc/ -nostdinc
-B/users/joel/test-gcc/b-arm-eabi-gcc/arm-eabi/newlib/ -isystem
/users/joel/test-gcc/b-arm-eabi-gcc/arm-eabi/newlib/targ-include
-isystem /users/joel/test-gcc/gcc/newlib/libc/include
-B/users/joel/test-gcc/b-arm-eabi-gcc/arm-eabi/libgloss/arm
-L/users/joel/test-gcc/b-arm-eabi-gcc/arm-eabi/libgloss/libnosys
-L/users/joel/test-gcc/gcc/libgloss/arm
-B/users/joel/test-gcc/install-head/arm-eabi/bin/
-B/users/joel/test-gcc/install-head/arm-eabi/lib/ -isystem
/users/joel/test-gcc/install-head/arm-eabi/include -isystem
/users/joel/test-gcc/install-head/arm-eabi/sys-include-g -O2 -mthumb
-O2  -g -O2 -DIN_GCC  -DCROSS_DIRECTORY_STRUCTURE  -W -Wall
-Wno-narrowing -Wwrite-strings -Wcast-qual -Wstrict-prototypes
-Wmissing-prototypes -Wold-style-definition  -isystem ./include  
-fno-inline -g -DIN_LIBGCC2 -fbuilding-libgcc -fno-stack-protector
-Dinhibit_libc  -fno-inline -I. -I. -I../../.././gcc
-I../../../../gcc/libgcc -I../../../../gcc/libgcc/.
-I../../../../gcc/libgcc/../gcc -I../../../../gcc/libgcc/../include 
-DHAVE_CC_TLS  -o _muldi3.o -MT _muldi3.o -MD -MP -MF _muldi3.dep
-DL_muldi3 -c ../../../../gcc/libgcc/libgcc2.c -fvisibility=hidden
-DHIDE_EXPORTS
/tmp/cc9EfnXy.s: Assembler messages:
/tmp/cc9EfnXy.s:69: Error: MOV Rd, Rs with two low registers is not
permitted on this architecture -- `mov r6,r7'

-- 
Joel Sherrill, Ph.D. Director of Research & Development
joel.sherr...@oarcorp.comOn-Line Applications Research
Ask me about RTEMS: a free RTOS  Huntsville AL 35805
Support Available(256) 722-9985

Re: testing policy for C/C++ front end changes

[Sandra Loosemore]

On 11/10/2014 05:03 AM, Richard Biener wrote:

On Mon, Nov 10, 2014 at 5:50 AM, Jeff Law  wrote:

On 11/09/14 16:13, Sandra Loosemore wrote:


https://gcc.gnu.org/contribute.html#testing

and noticed that the policy is to require a complete bootstrap for C
changes, but not for C++.  Given that GCC's implementation language is
now C++, isn't that backwards?  I'm not trying to weasel out of the
extra work for my patch, just curious if the web site guidelines have
gotten bit-rotten after the switch to C++, or if the SC did indeed
consider the issue already and the published policy is accurate.


They've bit-rotted a bit.  Interested in cons-ing up an update?


Bootstrap should now be required for both C and C++ FE changes
_and_ for libstdc++ changes as well given we start to pull in
libstdc++ headers during bootstrap.


H.  How about the attached patch?  I also added a blurb about 
target-specific patches that I think reflects current practice.


-Sandra

? htdocs/contribute.html.~1.85.~
Index: htdocs/contribute.html
===
RCS file: /cvs/gcc/wwwdocs/htdocs/contribute.html,v
retrieving revision 1.85
diff -u -p -r1.85 contribute.html
--- htdocs/contribute.html	27 Jun 2014 11:12:18 -	1.85
+++ htdocs/contribute.html	10 Nov 2014 15:19:37 -
@@ -100,9 +100,13 @@ list.
 Which tests to perform
 
 If your change is to code that is not in a front end, or is to the
-C front end, you must perform a complete build of GCC and the runtime
+C or C++ front ends or libgcc or libstdc++
+libraries, you must perform a complete build of GCC and the runtime
 libraries included with it, on at least one target.  You must
-bootstrap all default languages, not just C, and run all testsuites.
+build all default languages, not just C, and run all testsuites.
+If your change is specific to a particular target back end, you need only
+build and test that target; otherwise a complete bootstrap on a 
+primary platform designated in the current release criteria is required.
 For a normal native configuration, running
 
 make bootstrap
@@ -111,19 +115,9 @@ make -k check
 from the top level of the GCC tree (not the
 gcc subdirectory) will accomplish this.
 
-If your change is to the C++ front end, you should rebuild the compiler,
-libstdc++, libjava and run the C++ testsuite.
-If you already did a complete C,C++,Java bootstrap from your build
-directory before, you can use the following:
-
-make clean-target-libstdc++-v3# clean libstdc++ and ...
-test -d */libjava && make -C */libjava clean-nat  # ... parts of libjava
-make all-target-libstdc++-v3 all-target-libjava   # rebuild compiler and libraries
-make -k check-c++ # run C++/libstdc++ testsuite
-
-
 If your change is to a front end other than the C or C++ front end,
-or a runtime library other than libgcc, you need to verify
+or a runtime library other than libgcc or
+libstdc++, you need to verify
 only that the runtime library for that language still builds and the
 tests for that language have not regressed.  (Most languages have
 tests stored both in the gcc subdirectory, and in the

Re: mt_allocator.cc assumes sizeof(size_t) == sizeof(void *)

[Joseph Myers]

On Sat, 8 Nov 2014, Paolo Carlini wrote:

> Good. Sorry, if I missed some relatively recent development: is now GCC
> installing its own stdint.h on *all* the supported targets, thus we can safely

No; I sent a list of targets missing the information in 
 (possibly out 
of date now, but the goal there of unifying uint32_type_node and 
c_uint32_type_node etc. still applies).

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: mt_allocator.cc assumes sizeof(size_t) == sizeof(void *)

[Joel Sherrill]

On 11/10/2014 10:32 AM, Joseph Myers wrote:
> On Sat, 8 Nov 2014, Paolo Carlini wrote:
>
>> Good. Sorry, if I missed some relatively recent development: is now GCC
>> installing its own stdint.h on *all* the supported targets, thus we can 
>> safely
> No; I sent a list of targets missing the information in 
>  (possibly out 
> of date now, but the goal there of unifying uint32_type_node and 
> c_uint32_type_node etc. still applies).
>
I just submitted a patch using stdint.h and uintptr_t to gcc-patches.
I think this fixes the code in a standard way. I don't know what to
do about the list of targets you cited which don't support that.

> We are, it appears, still missing stdint.h type information in GCC for 
> NetBSD, VxWorks, Symbian, LynxOS, QNX, Interix, TPF.  Maybe we need to 
> issue a last call to maintainers caring about those targets to fill in 
> this information, and failing that deprecate them.

-- 
Joel Sherrill, Ph.D. Director of Research & Development
joel.sherr...@oarcorp.comOn-Line Applications Research
Ask me about RTEMS: a free RTOS  Huntsville AL 35805
Support Available(256) 722-9985

Re: mt_allocator.cc assumes sizeof(size_t) == sizeof(void *)

[Paolo Carlini]

Hi,

On 11/10/2014 05:49 PM, Joel Sherrill wrote:

On 11/10/2014 10:32 AM, Joseph Myers wrote:

On Sat, 8 Nov 2014, Paolo Carlini wrote:


Good. Sorry, if I missed some relatively recent development: is now GCC
installing its own stdint.h on *all* the supported targets, thus we can safely

No; I sent a list of targets missing the information in
 (possibly out
of date now, but the goal there of unifying uint32_type_node and
c_uint32_type_node etc. still applies).


I just submitted a patch using stdint.h and uintptr_t to gcc-patches.
I think this fixes the code in a standard way. I don't know what to
do about the list of targets you cited which don't support that.
If you unconditionally use stdint.h facilities you are going to break 
the build on other systems, those missing stdint.h. Note that, assuming 
you want to stick with this sort of general approach, you don't really 
need to use uintptr_t, you can for example fiddle a bit with templates 
and figure out on the fly at compile time an unsigned integer type large 
enough to hold a pointer (for example, what I quickly put together a few 
years ago toward the end of locale_facets.tcc, in do_put, appears to 
work decently well)


Paolo.

Re: mt_allocator.cc assumes sizeof(size_t) == sizeof(void *)

[Joseph Myers]

On Mon, 10 Nov 2014, Joel Sherrill wrote:

> On 11/10/2014 10:32 AM, Joseph Myers wrote:
> > On Sat, 8 Nov 2014, Paolo Carlini wrote:
> >
> >> Good. Sorry, if I missed some relatively recent development: is now GCC
> >> installing its own stdint.h on *all* the supported targets, thus we can 
> >> safely
> > No; I sent a list of targets missing the information in 
> >  (possibly out 
> > of date now, but the goal there of unifying uint32_type_node and 
> > c_uint32_type_node etc. still applies).
> >
> I just submitted a patch using stdint.h and uintptr_t to gcc-patches.
> I think this fixes the code in a standard way. I don't know what to
> do about the list of targets you cited which don't support that.

Well, some of them may in fact have their own stdint.h provided by the OS, 
or not use libstdc++, but if they use libstdc++ and don't have stdint.h, 
the correct fix is for the OS maintainer to add the necessary GCC support 
for providing stdint.h.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: mt_allocator.cc assumes sizeof(size_t) == sizeof(void *)

[Joel Sherrill]

On 11/10/2014 10:59 AM, Joseph Myers wrote:
> On Mon, 10 Nov 2014, Joel Sherrill wrote:
>
>> On 11/10/2014 10:32 AM, Joseph Myers wrote:
>>> On Sat, 8 Nov 2014, Paolo Carlini wrote:
>>>
 Good. Sorry, if I missed some relatively recent development: is now GCC
 installing its own stdint.h on *all* the supported targets, thus we can 
 safely
>>> No; I sent a list of targets missing the information in 
>>>  (possibly out 
>>> of date now, but the goal there of unifying uint32_type_node and 
>>> c_uint32_type_node etc. still applies).
>>>
>> I just submitted a patch using stdint.h and uintptr_t to gcc-patches.
>> I think this fixes the code in a standard way. I don't know what to
>> do about the list of targets you cited which don't support that.
> Well, some of them may in fact have their own stdint.h provided by the OS, 
> or not use libstdc++, but if they use libstdc++ and don't have stdint.h, 
> the correct fix is for the OS maintainer to add the necessary GCC support 
> for providing stdint.h.
>
I will leave it for you all to decide to push my patch or switch to
something like Paolo's trick.

-- 
Joel Sherrill, Ph.D. Director of Research & Development
joel.sherr...@oarcorp.comOn-Line Applications Research
Ask me about RTEMS: a free RTOS  Huntsville AL 35805
Support Available(256) 722-9985

Re: arm/thumb broken on head

[Richard Earnshaw]

On 10/11/14 15:26, Joel Sherrill wrote:
> Hi
> 
> With gcc, newlib, and binutils head, arm-rtems and arm-eabi both
> die building libgcc2.c for the Thumb. I don't know if this is a recent
> gcc change or binutils having added some new error checking. Anyone
> got any ideas?
> 
> /users/joel/test-gcc/b-arm-eabi-gcc/./gcc/xgcc
> -B/users/joel/test-gcc/b-arm-eabi-gcc/./gcc/ -nostdinc
> -B/users/joel/test-gcc/b-arm-eabi-gcc/arm-eabi/newlib/ -isystem
> /users/joel/test-gcc/b-arm-eabi-gcc/arm-eabi/newlib/targ-include
> -isystem /users/joel/test-gcc/gcc/newlib/libc/include
> -B/users/joel/test-gcc/b-arm-eabi-gcc/arm-eabi/libgloss/arm
> -L/users/joel/test-gcc/b-arm-eabi-gcc/arm-eabi/libgloss/libnosys
> -L/users/joel/test-gcc/gcc/libgloss/arm
> -B/users/joel/test-gcc/install-head/arm-eabi/bin/
> -B/users/joel/test-gcc/install-head/arm-eabi/lib/ -isystem
> /users/joel/test-gcc/install-head/arm-eabi/include -isystem
> /users/joel/test-gcc/install-head/arm-eabi/sys-include-g -O2 -mthumb
> -O2  -g -O2 -DIN_GCC  -DCROSS_DIRECTORY_STRUCTURE  -W -Wall
> -Wno-narrowing -Wwrite-strings -Wcast-qual -Wstrict-prototypes
> -Wmissing-prototypes -Wold-style-definition  -isystem ./include  
> -fno-inline -g -DIN_LIBGCC2 -fbuilding-libgcc -fno-stack-protector
> -Dinhibit_libc  -fno-inline -I. -I. -I../../.././gcc
> -I../../../../gcc/libgcc -I../../../../gcc/libgcc/.
> -I../../../../gcc/libgcc/../gcc -I../../../../gcc/libgcc/../include 
> -DHAVE_CC_TLS  -o _muldi3.o -MT _muldi3.o -MD -MP -MF _muldi3.dep
> -DL_muldi3 -c ../../../../gcc/libgcc/libgcc2.c -fvisibility=hidden
> -DHIDE_EXPORTS
> /tmp/cc9EfnXy.s: Assembler messages:
> /tmp/cc9EfnXy.s:69: Error: MOV Rd, Rs with two low registers is not
> permitted on this architecture -- `mov r6,r7'
> 

This is most likely fallout from the migration of thumb1 assembly to
unified syntax.  Terry, if this hasn't already been sorted can you take
a look?

R.

Re: mt_allocator.cc assumes sizeof(size_t) == sizeof(void *)

[Jonathan Wakely]

On 10 November 2014 16:49, Joel Sherrill wrote:
> I just submitted a patch using stdint.h and uintptr_t to gcc-patches.

libstdc++ patches must be CCd to the libstdc++ list.

How is libtool updated in GCC?

[FX]

Hi,

libtool just released a new version to add support for OS X Yosemite (the old 
libtool had a poorly designed globbing pattern). It really is a one-line change 
(see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63610#c7), but we need to 
incorporate it into the GCC code base and rebuild all our configure’s.

How is libtool handled in GCC? Do we import from upstream, or merge the changes 
that we need? It looks like it’s done manually and selectively, but I’d like 
confirmation of that.

Thanks,
FX

Re: Match-and-simplify and COND_EXPR

[Andrew Pinski]

On Fri, Nov 7, 2014 at 2:23 AM, Richard Biener  wrote:
> On Thu, 6 Nov 2014, Andrew Pinski wrote:
>
>> On Thu, Nov 6, 2014 at 2:40 AM, Richard Biener  wrote:
>> > On Thu, 6 Nov 2014, Richard Biener wrote:
>> >
>> >> On Wed, 5 Nov 2014, Andrew Pinski wrote:
>> >>
>> >> > Hi,
>> >> >   I was trying to hook up tree-ssa-phiopt to match-and-simplify using
>> >> > either gimple_build (or rather using gimple_simplify depending on if
>> >> > we want to produce cond_expr for conditional move).  I ran into a
>> >> > problem.
>> >> > With the pattern below:
>> >> > /* a ? 0 : 1 -> a if 0 and 1 are integral types. */
>> >> > (simplify
>> >> >   (cond_expr @0 integer_zerop integer_onep)
>> >> >   (if (INTEGRAL_TYPE_P (type))
>> >> > (convert @0)))
>> >>
>> >> Ok, so you are capturing a GENERIC expr here but nothing knows that.
>> >> It would work if you'd do (ugh)
>> >>
>> >> (for op (lt le eq ne ge gt)
>> >>  (simplify
>> >>   (cond_expr (op @0 @1) integer_zerop integer_onep)
>> >>   (if (INTEGRAL_TYPE_P (type))
>> >>(convert (op @0 @1)
>> >> (simplify
>> >>  (cond_expr SSA_NAME@0 integer_zerop integer_onep)
>> >>   (if (INTEGRAL_TYPE_P (type))
>> >>(convert @0
>> >>
>> >> as a workaround.  To make your version work will require (quite)
>> >> some special-casing in the code generator or maybe the resimplify
>> >> helper.  Let me see if I can cook up a "simple" fix.
>> >
>> > Sth like below (for the real fix this has to be replicated in
>> > all gimple_resimplifyN functions).  I'm missing a testcase
>> > where the pattern would apply (and not be already folded by fold),
>> > so I didn't check if it actually works.
>>
>> You do need to check if seq is NULL though as gimple_build depends on
>> seq not being NULL.  But otherwise yes this works for me.
>
> Ok, here is a better and more complete fix.  The optimization
> whether a captured expression may be a GENERIC condition isn't
> implemented so a lot of checks are emitted right now.  Also
> if gimple_build would fail this terminates the whole matching
> process, not just matching of the current pattern (failing "late"
> isn't supported with the style of code we emit - well, without
> adding ugly gotos and labels).
>
> At least it avoids splitting up conditions if substituted into
> a COND_EXPR, so simplifications of COND_EXPRs in-place (w/o seq)
> should still work.
>
> Can you check whether this works for you?

This works for most cases but does not work for things like:
/* a != b ? a : b -> a */
(simplify
  (cond_expr (ne @0 @1) @0 @1)
  @0)

In gimple_simplify after it matches COND_EXPR. It does not look into
the operand 0 to see if it matches NE_EXPR, it just sees if it was a
SSA_NAME.  In this case I was trying to do a simple replacement of
value_replacement in tree-ssa-phiopt.c.

But for my purpose right now this is sufficient and will be posting a
patch which does not remove things from tree-ssa-phiopt.c just yet.

Thanks,
Andrew Pinski


>
> Thanks,
> Richard.
>
>
>
> Index: gcc/genmatch.c
> ===
> --- gcc/genmatch.c  (revision 217213)
> +++ gcc/genmatch.c  (working copy)
> @@ -419,7 +419,8 @@ struct operand {
>operand (enum op_type type_) : type (type_) {}
>enum op_type type;
>virtual void gen_transform (FILE *, const char *, bool, int,
> - const char *, dt_operand ** = 0)
> + const char *, dt_operand ** = 0,
> + bool = true)
>  { gcc_unreachable  (); }
>  };
>
> @@ -449,7 +450,7 @@ struct expr : public operand
>   later lowered to two separate patterns.  */
>bool is_commutative;
>virtual void gen_transform (FILE *f, const char *, bool, int,
> - const char *, dt_operand ** = 0);
> + const char *, dt_operand ** = 0, bool = true);
>  };
>
>  /* An operator that is represented by native C code.  This is always
> @@ -479,7 +480,7 @@ struct c_expr : public operand
>/* The identifier replacement vector.  */
>vec ids;
>virtual void gen_transform (FILE *f, const char *, bool, int,
> - const char *, dt_operand **);
> + const char *, dt_operand ** = 0, bool = true);
>  };
>
>  /* A wrapper around another operand that captures its value.  */
> @@ -493,7 +494,7 @@ struct capture : public operand
>/* The captured value.  */
>operand *what;
>virtual void gen_transform (FILE *f, const char *, bool, int,
> - const char *, dt_operand ** = 0);
> + const char *, dt_operand ** = 0, bool = true);
>  };
>
>  template<>
> @@ -1358,7 +1359,7 @@ get_operand_type (id_base *op, const cha
>
>  void
>  expr::gen_transform (FILE *f, const char *dest, bool gimple, int depth,
> -const char *in_type, dt_operand **indexes)
> +const char *in_type, dt_operand **indexes,

Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign

[David Malcolm]

On Sat, 2014-11-08 at 14:56 +0100, Jakub Jelinek wrote:
> On Sat, Nov 08, 2014 at 01:07:28PM +0100, Richard Biener wrote:
> > To be constructive here - the above case is from within a
> > GIMPLE_ASSIGN case label
> > and thus I'd have expected
> > 
> > case GIMPLE_ASSIGN:
> >   {
> > gassign *a1 = as_a  (s1);
> > gassign *a2 = as_a  (s2);
> >   lhs1 = gimple_assign_lhs (a1);
> >   lhs2 = gimple_assign_lhs (a2);
> >   if (TREE_CODE (lhs1) != SSA_NAME
> >   && TREE_CODE (lhs2) != SSA_NAME)
> > return (operand_equal_p (lhs1, lhs2, 0)
> > && gimple_operand_equal_value_p (gimple_assign_rhs1 (a1),
> >  gimple_assign_rhs1 (a2)));
> >   else if (TREE_CODE (lhs1) == SSA_NAME
> >&& TREE_CODE (lhs2) == SSA_NAME)
> > return vn_valueize (lhs1) == vn_valueize (lhs2);
> >   return false;
> >   }
> > 
> > instead.  That's the kind of changes I have expected and have approved of.
> 
> But even that looks like just adding extra work for all developers, with no
> gain.  You only have to add extra code and extra temporaries, in switches
> typically also have to add {} because of the temporaries and thus extra
> indentation level, and it doesn't simplify anything in the code.

The branch attempts to use the C++ typesystem to capture information
about the kinds of gimple statement we expect, both:
  (A) so that the compiler can detect type errors, and
  (B) as a comprehension aid to the human reader of the code

The ideal here is when function params and struct field can be
strengthened from "gimple" to a subclass ptr.  This captures the
knowledge that every use of a function or within a struct has a given
gimple code.

Examples of this for the initial patchkit were:

* the "call_stmt" field of a cgraph_edge becoming a gcall *,
  rather than a plain gimple.

* various variables in tree-into-ssa.c change from just
  vec to being vec, capturing the "phi-ness" of
  the contents as a compile-time check

* tree-inline.h's struct copy_body_data, the field "debug_stmts"
  can be "concretized" from a vec to a vec.

A more recent example, from:
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=5bd16d92b9e928b5a5a7aebd571d92f72dd517f8
The fields "arr_ref_first" and "arr_ref_last" of
tree-switch-conversion.c's struct switch_conv_info can be strengthened
from gimple to gassign *: they are always GIMPLE_ASSIGN.

I applied cleanups to do my initial patchkit, which Jeff approved (with
some provisos), and which became the first 92 commits on the branch:

"[gimple-classes, committed 00/92] Initial slew of commits":
  https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02791.html

followed by a merger from trunk into the branch:
"[gimple-classes] Merge trunk r216157-r216746 into branch":
  https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02982.html

With those commits, I was able to convert 180 accessors to taking a
concrete subclass, with 158 left taking a gimple or
const_gimple i.e. about half of them.
(My script to analyze this is gimple_typesafety.py
within https://github.com/davidmalcolm/gcc-refactoring-scripts)

I got it into my head that it was desirable to convert *all*
gimple accessors to this form, and to eliminate the GIMPLE_CHECK
macros (given that gcc development community seems to dislike
partial transitions).

I've been attempting this full conversion - convert all of the
gimple_ accessors, to require an appropriate gimple subclass
ptr, in particular focusing on the gimple_assign_ ones, but it's a *lot*
of extra work, and much more invasive than the patches
that Jeff conditionally approved.

I now suspect that it's going too far - in the initial patchkit I was
doing the clean, obvious ones, but now I'm left with the awkward ones
that would require me to uglify the code to "fix".

If it's OK to only convert some of them, then I'd rather just do that.

The type-strengthening is rarely as neat as being able to simply convert
a param or field type.  Some examples:

Functions passed a gsi
==
Sometimes functions are passed a gsi, where it can be known that the gsi
currently references a stmt of known kind (although that isn't
necessarily obvious from reading the body of the function):

Example from tree-ssa-strlen.c:

handle_char_store (gimple_stmt_iterator *gsi)
 {
   int idx = -1;
   strinfo si = NULL;
-  gimple stmt = gsi_stmt (*gsi);
+  gassign *stmt = as_a  (gsi_stmt (*gsi));
   tree ssaname = NULL_TREE, lhs = gimple_assign_lhs (stmt);
 
   if (TREE_CODE (lhs) == MEM_REF

from
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=78aae552f15ad5f8f5290fb825f9ae33f4a7cad9


Only acting on one kind of gimple
=

Some functions accept any kind of gimple, but only act on e.g. a
GIMPLE_ASSIGN, immediately returning if they got a different kind.

So I make this kind of change, where:

  void
  foo (gimple stmt, other params)
  {
if (!is_gimple_assi

Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign

[Andrew Pinski]

On Mon, Nov 10, 2014 at 2:27 PM, David Malcolm  wrote:
> On Sat, 2014-11-08 at 14:56 +0100, Jakub Jelinek wrote:
>> On Sat, Nov 08, 2014 at 01:07:28PM +0100, Richard Biener wrote:
>> > To be constructive here - the above case is from within a
>> > GIMPLE_ASSIGN case label
>> > and thus I'd have expected
>> >
>> > case GIMPLE_ASSIGN:
>> >   {
>> > gassign *a1 = as_a  (s1);
>> > gassign *a2 = as_a  (s2);
>> >   lhs1 = gimple_assign_lhs (a1);
>> >   lhs2 = gimple_assign_lhs (a2);
>> >   if (TREE_CODE (lhs1) != SSA_NAME
>> >   && TREE_CODE (lhs2) != SSA_NAME)
>> > return (operand_equal_p (lhs1, lhs2, 0)
>> > && gimple_operand_equal_value_p (gimple_assign_rhs1 (a1),
>> >  gimple_assign_rhs1 (a2)));
>> >   else if (TREE_CODE (lhs1) == SSA_NAME
>> >&& TREE_CODE (lhs2) == SSA_NAME)
>> > return vn_valueize (lhs1) == vn_valueize (lhs2);
>> >   return false;
>> >   }
>> >
>> > instead.  That's the kind of changes I have expected and have approved of.
>>
>> But even that looks like just adding extra work for all developers, with no
>> gain.  You only have to add extra code and extra temporaries, in switches
>> typically also have to add {} because of the temporaries and thus extra
>> indentation level, and it doesn't simplify anything in the code.
>
> The branch attempts to use the C++ typesystem to capture information
> about the kinds of gimple statement we expect, both:
>   (A) so that the compiler can detect type errors, and
>   (B) as a comprehension aid to the human reader of the code
>
> The ideal here is when function params and struct field can be
> strengthened from "gimple" to a subclass ptr.  This captures the
> knowledge that every use of a function or within a struct has a given
> gimple code.
>
> Examples of this for the initial patchkit were:
>
> * the "call_stmt" field of a cgraph_edge becoming a gcall *,
>   rather than a plain gimple.
>
> * various variables in tree-into-ssa.c change from just
>   vec to being vec, capturing the "phi-ness" of
>   the contents as a compile-time check
>
> * tree-inline.h's struct copy_body_data, the field "debug_stmts"
>   can be "concretized" from a vec to a vec.
>
> A more recent example, from:
> https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=5bd16d92b9e928b5a5a7aebd571d92f72dd517f8
> The fields "arr_ref_first" and "arr_ref_last" of
> tree-switch-conversion.c's struct switch_conv_info can be strengthened
> from gimple to gassign *: they are always GIMPLE_ASSIGN.
>
> I applied cleanups to do my initial patchkit, which Jeff approved (with
> some provisos), and which became the first 92 commits on the branch:
>
> "[gimple-classes, committed 00/92] Initial slew of commits":
>   https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02791.html
>
> followed by a merger from trunk into the branch:
> "[gimple-classes] Merge trunk r216157-r216746 into branch":
>   https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02982.html
>
> With those commits, I was able to convert 180 accessors to taking a
> concrete subclass, with 158 left taking a gimple or
> const_gimple i.e. about half of them.
> (My script to analyze this is gimple_typesafety.py
> within https://github.com/davidmalcolm/gcc-refactoring-scripts)
>
> I got it into my head that it was desirable to convert *all*
> gimple accessors to this form, and to eliminate the GIMPLE_CHECK
> macros (given that gcc development community seems to dislike
> partial transitions).
>
> I've been attempting this full conversion - convert all of the
> gimple_ accessors, to require an appropriate gimple subclass
> ptr, in particular focusing on the gimple_assign_ ones, but it's a *lot*
> of extra work, and much more invasive than the patches
> that Jeff conditionally approved.
>
> I now suspect that it's going too far - in the initial patchkit I was
> doing the clean, obvious ones, but now I'm left with the awkward ones
> that would require me to uglify the code to "fix".
>
> If it's OK to only convert some of them, then I'd rather just do that.
>
> The type-strengthening is rarely as neat as being able to simply convert
> a param or field type.  Some examples:
>
> Functions passed a gsi
> ==
> Sometimes functions are passed a gsi, where it can be known that the gsi
> currently references a stmt of known kind (although that isn't
> necessarily obvious from reading the body of the function):
>
> Example from tree-ssa-strlen.c:
>
> handle_char_store (gimple_stmt_iterator *gsi)
>  {
>int idx = -1;
>strinfo si = NULL;
> -  gimple stmt = gsi_stmt (*gsi);
> +  gassign *stmt = as_a  (gsi_stmt (*gsi));


Can we have something like:
gsi_assign (*gsi)
instead so there is less typing when we want an gassign rather than a
gimple stmt.  This should allow for easier converting also and puts
most of the as_a in one place.

Thanks,
Andrew Pinski

>tree ssaname = NULL_TREE, lhs = gimple

RE: arm/thumb broken on head

[Terry Guo]

> -Original Message-
> From: Richard Earnshaw
> Sent: Tuesday, November 11, 2014 1:08 AM
> To: Joel Sherrill; GCC Mailing List
> Cc: Terry Guo
> Subject: Re: arm/thumb broken on head
> 
> On 10/11/14 15:26, Joel Sherrill wrote:
> > Hi
> >
> > With gcc, newlib, and binutils head, arm-rtems and arm-eabi both die
> > building libgcc2.c for the Thumb. I don't know if this is a recent gcc
> > change or binutils having added some new error checking. Anyone got
> > any ideas?
> >
> > /users/joel/test-gcc/b-arm-eabi-gcc/./gcc/xgcc
> > -B/users/joel/test-gcc/b-arm-eabi-gcc/./gcc/ -nostdinc
> > -B/users/joel/test-gcc/b-arm-eabi-gcc/arm-eabi/newlib/ -isystem
> > /users/joel/test-gcc/b-arm-eabi-gcc/arm-eabi/newlib/targ-include
> > -isystem /users/joel/test-gcc/gcc/newlib/libc/include
> > -B/users/joel/test-gcc/b-arm-eabi-gcc/arm-eabi/libgloss/arm
> > -L/users/joel/test-gcc/b-arm-eabi-gcc/arm-eabi/libgloss/libnosys
> > -L/users/joel/test-gcc/gcc/libgloss/arm
> > -B/users/joel/test-gcc/install-head/arm-eabi/bin/
> > -B/users/joel/test-gcc/install-head/arm-eabi/lib/ -isystem
> > /users/joel/test-gcc/install-head/arm-eabi/include -isystem
> > /users/joel/test-gcc/install-head/arm-eabi/sys-include-g -O2 -mthumb
> > -O2  -g -O2 -DIN_GCC  -DCROSS_DIRECTORY_STRUCTURE  -W -Wall
> > -Wno-narrowing -Wwrite-strings -Wcast-qual -Wstrict-prototypes
> > -Wmissing-prototypes -Wold-style-definition  -isystem ./include
> > -fno-inline -g -DIN_LIBGCC2 -fbuilding-libgcc -fno-stack-protector
> > -Dinhibit_libc  -fno-inline -I. -I. -I../../.././gcc
> > -I../../../../gcc/libgcc -I../../../../gcc/libgcc/.
> > -I../../../../gcc/libgcc/../gcc -I../../../../gcc/libgcc/../include
> > -DHAVE_CC_TLS  -o _muldi3.o -MT _muldi3.o -MD -MP -MF _muldi3.dep
> > -DL_muldi3 -c ../../../../gcc/libgcc/libgcc2.c -fvisibility=hidden
> > -DHIDE_EXPORTS
> > /tmp/cc9EfnXy.s: Assembler messages:
> > /tmp/cc9EfnXy.s:69: Error: MOV Rd, Rs with two low registers is not
> > permitted on this architecture -- `mov r6,r7'
> >
> 
> This is most likely fallout from the migration of thumb1 assembly to
unified
> syntax.  Terry, if this hasn't already been sorted can you take a look?
> 
> R.

Sorry for troubles. Indeed this is caused my recent Thumb-1 UAL patch. Some
'mov' instructions require special treatment. I am working on a patch.

BR,
Terry

Re: How is libtool updated in GCC?

[Joseph Myers]

On Mon, 10 Nov 2014, FX wrote:

> How is libtool handled in GCC? Do we import from upstream, or merge the 
> changes that we need? It looks like it’s done manually and selectively, 
> but I’d like confirmation of that.

If you import rather than selectively merging one change you need (I 
think) to revert libtool commit 3334f7ed5851ef1e96b052f2984c4acdbf39e20c 
(incompatible with GCC handling of sysroots), as well as updating all 
three relevant repositories (GCC, binutils-gdb, src - I'm not sure if 
anything in binutils-gdb actually uses libtool but it has the files at 
toplevel).

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign

[Jakub Jelinek]

On Mon, Nov 10, 2014 at 05:27:50PM -0500, David Malcolm wrote:
> On Sat, 2014-11-08 at 14:56 +0100, Jakub Jelinek wrote:
> > On Sat, Nov 08, 2014 at 01:07:28PM +0100, Richard Biener wrote:
> > > To be constructive here - the above case is from within a
> > > GIMPLE_ASSIGN case label
> > > and thus I'd have expected
> > > 
> > > case GIMPLE_ASSIGN:
> > >   {
> > > gassign *a1 = as_a  (s1);
> > > gassign *a2 = as_a  (s2);
> > >   lhs1 = gimple_assign_lhs (a1);
> > >   lhs2 = gimple_assign_lhs (a2);
> > >   if (TREE_CODE (lhs1) != SSA_NAME
> > >   && TREE_CODE (lhs2) != SSA_NAME)
> > > return (operand_equal_p (lhs1, lhs2, 0)
> > > && gimple_operand_equal_value_p (gimple_assign_rhs1 (a1),
> > >  gimple_assign_rhs1 
> > > (a2)));
> > >   else if (TREE_CODE (lhs1) == SSA_NAME
> > >&& TREE_CODE (lhs2) == SSA_NAME)
> > > return vn_valueize (lhs1) == vn_valueize (lhs2);
> > >   return false;
> > >   }
> > > 
> > > instead.  That's the kind of changes I have expected and have approved of.
> > 
> > But even that looks like just adding extra work for all developers, with no
> > gain.  You only have to add extra code and extra temporaries, in switches
> > typically also have to add {} because of the temporaries and thus extra
> > indentation level, and it doesn't simplify anything in the code.
> 
> The branch attempts to use the C++ typesystem to capture information
> about the kinds of gimple statement we expect, both:
>   (A) so that the compiler can detect type errors, and
>   (B) as a comprehension aid to the human reader of the code
> 
> The ideal here is when function params and struct field can be
> strengthened from "gimple" to a subclass ptr.  This captures the
> knowledge that every use of a function or within a struct has a given
> gimple code.

I just don't like all the as_a/is_a stuff enforced everywhere,
it means more typing, more temporaries, more indentation.
So, as I view it, instead of the checks being done cheaply (yes, I think
the gimple checking as we have right now is very cheap) under the
hood by the accessors (gimple_assign_{lhs,rhs1} etc.), those changes
put the burden on the developers, who has to check that manually through
the as_a/is_a stuff everywhere, more typing and uglier syntax.
I just don't see that as a step forward, instead a huge step backwards.
But perhaps I'm alone with this.
Can you e.g. compare the size of - lines in your patchset combined, and
size of + lines in your patchset?  As in, if your changes lead to less
typing or more.

Jakub