Re: [PATCH] Fix PR81921

[Joseph Myers]

I'm seeing a build failure for s390x-linux-gnu that looks like it could be 
related to this change (build was OK at r251332, failed at r251358).

https://sourceware.org/ml/libc-testresults/2017-q3/msg00329.html

In file included from /scratch/jmyers/glibc-bot/src/gcc/libitm/libitm_i.h:39:0,
 from /scratch/jmyers/glibc-bot/src/gcc/libitm/beginend.cc:25:
/scratch/jmyers/glibc-bot/src/gcc/libitm/local_atomic: In static member 
function 'static uint32_t GTM::gtm_thread::begin_transaction(uint32_t, const 
gtm_jmpbuf*)':
/scratch/jmyers/glibc-bot/src/gcc/libitm/local_atomic:589:7: error: inlining 
failed in call to always_inline 'std::__atomic_base<_IntTp>::__int_type 
std::__atomic_base<_IntTp>::fetch_add(std::__atomic_base<_IntTp>::__int_type, 
std::memory_order) noexcept [with _ITp = long unsigned int]': target specific 
option mismatch
   fetch_add(__int_type __i,
   ^
/scratch/jmyers/glibc-bot/src/gcc/libitm/beginend.cc:395:36: note: called from 
here
   tx->id = global_tid.fetch_add(tid_block_size, memory_order_relaxed);
^~
Makefile:546: recipe for target 'beginend.lo' failed
make[5]: *** [beginend.lo] Error 1

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

Re: [1/2] PR 78736: New warning -Wenum-conversion

[Joseph Myers]

On Tue, 11 Jul 2017, Prathamesh Kulkarni wrote:

> On 13 June 2017 at 01:47, Joseph Myers  wrote:
> > This is OK with one fix:
> >
> >> +C ObjC Var(warn_enum_conversion) Init(0) Warning LangEnabledBy(C 
> >> Objc,Wall)
> >
> > I believe the LangEnabledBy arguments are case-sensitive, so you need to
> > have ObjC not Objc there for it to work correctly.  (*.opt parsing isn't
> > very good at detecting typos and giving errors rather than silently
> > ignoring things.)
> Hi,
> Sorry for the late response, I was on a vacation.
> The attached patch is rebased and bootstrap+tested on 
> x86_64-unknown-linux-gnu.
> I have modified it slightly to not warn for enums with different names
> but having same value ranges.
> For eg:
> enum e1 { e1_1, e1_2 };
> enum e2 { e2_1, e2_2 };
> 
> enum e1 x = e2_1;
> With this version, there would be no warning for the above assignment
> since both e1 and e2 have
> same value ranges.  Is that OK ?

I don't really think that's a good heuristic.  I see no particular reason 
to think that just because two enums have the same set of values, an 
implicit conversion between them is actually deliberate - corresponding 
values have the same semantics in some sense - rather than reflecting an 
underlying confusion in the code.  (You could have different levels of the 
warning - and only warn in this case at a higher level - but I don't 
really think that makes sense either for this particular warning.)

> The patch has following fallouts in the testsuite:
> 
> a) libgomp:
> I initially assume it was a false positive because I thought enum
> gomp_schedule_type
> and enum omp_sched_t have same value-ranges but it looks like omp_sched_t
> has range [1, 4] while gomp_schedule_type has range [0, 4] with one
> extra element.
> Is the warning then correct for this case ?
> 
> b) libgfortran:
> i) Implicit conversion from unit_mode to file_mode
> ii) Implicit conversion from unit_sign_s to unit_sign.
> I suppose the warning is OK for these cases since unit_mode, file_mode
> have different value-ranges and similarly for unit_sign_s, unit_sign ?

If it's an implicit conversion between different enum types, the warning 
is correct.  The more important question for the review is: is it correct 
to replace the implicit conversion by an explicit one?  That is, for each 
value in the source type, what enumerator of the destination type has the 
same value, and do the semantics match in whatever way is required for the 
code in question?

Also note s/valeu/value/ in the documentation.

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

Re: [PATCH 1/2] Don't warn function alignment if warn_if_not_aligned_p is true

[H.J. Lu]

On Fri, Aug 25, 2017 at 2:38 PM, Martin Sebor <mse...@gmail.com> wrote:
> On 08/25/2017 03:05 PM, H.J. Lu wrote:
>>
>> On Fri, Aug 25, 2017 at 1:14 PM, Martin Sebor <mse...@gmail.com> wrote:
>>>
>>> On 08/25/2017 01:35 PM, H.J. Lu wrote:
>>>>
>>>>
>>>> On Fri, Aug 25, 2017 at 12:24 PM, Martin Sebor <mse...@gmail.com> wrote:
>>>>>
>>>>>
>>>>> On 08/25/2017 11:31 AM, H.J. Lu wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Fri, Aug 25, 2017 at 10:15 AM, Martin Sebor <mse...@gmail.com>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 08/21/2017 06:21 AM, H.J. Lu wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> When warn_if_not_aligned_p is true, a warning will be issued on
>>>>>>>> function
>>>>>>>> declaration later.  There is no need to warn function alignment when
>>>>>>>> warn_if_not_aligned_p is true.
>>>>>>>>
>>>>>>>> OK for trunk?
>>>>>>>>
>>>>>>>> H.J.
>>>>>>>> --
>>>>>>>> * c-attribs.c (common_handle_aligned_attribute): Don't warn
>>>>>>>> function alignment if warn_if_not_aligned_p is true.
>>>>>>>> ---
>>>>>>>>  gcc/c-family/c-attribs.c | 5 -
>>>>>>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
>>>>>>>> index 5f79468407f..78969532543 100644
>>>>>>>> --- a/gcc/c-family/c-attribs.c
>>>>>>>> +++ b/gcc/c-family/c-attribs.c
>>>>>>>> @@ -1754,9 +1754,12 @@ common_handle_aligned_attribute (tree *node,
>>>>>>>> tree
>>>>>>>> args, int flags,
>>>>>>>>This formally comes from the c++11 specification but we are
>>>>>>>>doing it for the GNU attribute syntax as well.  */
>>>>>>>>  *no_add_attrs = true;
>>>>>>>> -  else if (TREE_CODE (decl) == FUNCTION_DECL
>>>>>>>> +  else if (!warn_if_not_aligned_p
>>>>>>>> +  && TREE_CODE (decl) == FUNCTION_DECL
>>>>>>>>&& DECL_ALIGN (decl) > (1U << i) * BITS_PER_UNIT)
>>>>>>>>  {
>>>>>>>> +  /* Don't warn function alignment here if
>>>>>>>> warn_if_not_aligned_p
>>>>>>>> is
>>>>>>>> +true.  It will be warned later.  */
>>>>>>>>if (DECL_USER_ALIGN (decl))
>>>>>>>> error ("alignment for %q+D was previously specified as %d "
>>>>>>>>"and may not be decreased", decl,
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Your comment refers to warning but the code here uses error().
>>>>>>> That raises two questions for me: a) will the later diagnostic
>>>>>>> really be a warning or an error, and if a warning, under what
>>>>>>> option will it be issued? and b) why is an error appropriate
>>>>>>> here when a warning is appropriate elsewhere (most other
>>>>>>> attribute conflicts are at present diagnosed with -Wattributes).
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> It can be changed to warning.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Okay, that's goo to hear (it validates what I did in the patch
>>>>> I referenced).
>>>>>
>>>>> As for your patch, I don't quite see ho the block can ever be
>>>>> entered.  In fact, I had to make changes to the function in
>>>>> my patch below to let it detect and diagnose the condition
>>>>> (attempting to reduce the alignment of a function).  The
>>>>> details are in bug 81566.  Did I miss something?
>>>>
>>>>
>>>>
>>>> Try this:
>>>>
>>>> __attribute__((warn_if_not_aligned(8)))
>>>> void
>>>> foo2 (void)
>>>> {
>>>> }
>>>
>>>
>>>
>>> For me the top of trunk prints the same error both with and
>>> without your patch:
>>>
>>>   error: ‘warn_if_not_aligned’ may not be specified for ‘foo2’
>>
>>
>> Please try it with Linux/alpha cross compiler.
>
>
> I get the same error (both with and without your patch). Do
> I need to be using some specific options?  (-Wif-not-aligned
> makes no difference.)

Ooops.  It is ia64-linux:

[hjl@gnu-tools-1 gcc]$ cat /tmp/x.i
__attribute__((warn_if_not_aligned(8)))
void
foo2 (void)
{
}
[hjl@gnu-tools-1 gcc]$ ./xgcc -B./ /tmp/x.i -S -O2
/tmp/x.i:3:1: error: alignment for ‘foo2’ must be at least 16
 foo2 (void)
 ^~~~
/tmp/x.i:3:1: error: ‘warn_if_not_aligned’ may not be specified for ‘foo2’
[hjl@gnu-tools-1 gcc]$ cat x.i
__attribute__((aligned(32)))
void
foo2 (void)
{
}
[hjl@gnu-tools-1 gcc]$ ./xgcc -B./ -O2 x.i -S
[hjl@gnu-tools-1 gcc]$ cat x.s
.file "x.i"
.pred.safe_across_calls p1-p5,p16-p63
.text
.align 32 <<<<<<<<<<<<<< Aligned at 32 bytes.
.global foo2#
.type foo2#, @function
.proc foo2#
foo2:
.prologue
.body
.mib
nop 0
nop 0
br.ret.sptk.many b0
.endp foo2#
.ident "GCC: (GNU) 8.0.0 20170825 (experimental)"
[hjl@gnu-tools-1 gcc]$

> I'm also not sure I understand how the target affects this
> case (is there supposed to be some other attribute on the
> declaration above? attribute aligned).  I thought the
> handling of function declarations with the warn_if_not_aligned
> attribute was target independent.
>
> In any case, I think it would be easier if the patch included
> a test showing what the problem is and how the change prevents
> it.
>
> Martin



-- 
H.J.

Re: [PATCH 1/2] Don't warn function alignment if warn_if_not_aligned_p is true

[Martin Sebor]

On 08/25/2017 03:05 PM, H.J. Lu wrote:

On Fri, Aug 25, 2017 at 1:14 PM, Martin Sebor  wrote:

On 08/25/2017 01:35 PM, H.J. Lu wrote:


On Fri, Aug 25, 2017 at 12:24 PM, Martin Sebor  wrote:


On 08/25/2017 11:31 AM, H.J. Lu wrote:



On Fri, Aug 25, 2017 at 10:15 AM, Martin Sebor  wrote:



On 08/21/2017 06:21 AM, H.J. Lu wrote:




When warn_if_not_aligned_p is true, a warning will be issued on
function
declaration later.  There is no need to warn function alignment when
warn_if_not_aligned_p is true.

OK for trunk?

H.J.
--
* c-attribs.c (common_handle_aligned_attribute): Don't warn
function alignment if warn_if_not_aligned_p is true.
---
 gcc/c-family/c-attribs.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 5f79468407f..78969532543 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -1754,9 +1754,12 @@ common_handle_aligned_attribute (tree *node,
tree
args, int flags,
   This formally comes from the c++11 specification but we are
   doing it for the GNU attribute syntax as well.  */
 *no_add_attrs = true;
-  else if (TREE_CODE (decl) == FUNCTION_DECL
+  else if (!warn_if_not_aligned_p
+  && TREE_CODE (decl) == FUNCTION_DECL
   && DECL_ALIGN (decl) > (1U << i) * BITS_PER_UNIT)
 {
+  /* Don't warn function alignment here if warn_if_not_aligned_p
is
+true.  It will be warned later.  */
   if (DECL_USER_ALIGN (decl))
error ("alignment for %q+D was previously specified as %d "
   "and may not be decreased", decl,





Your comment refers to warning but the code here uses error().
That raises two questions for me: a) will the later diagnostic
really be a warning or an error, and if a warning, under what
option will it be issued? and b) why is an error appropriate
here when a warning is appropriate elsewhere (most other
attribute conflicts are at present diagnosed with -Wattributes).




It can be changed to warning.




Okay, that's goo to hear (it validates what I did in the patch
I referenced).

As for your patch, I don't quite see ho the block can ever be
entered.  In fact, I had to make changes to the function in
my patch below to let it detect and diagnose the condition
(attempting to reduce the alignment of a function).  The
details are in bug 81566.  Did I miss something?



Try this:

__attribute__((warn_if_not_aligned(8)))
void
foo2 (void)
{
}



For me the top of trunk prints the same error both with and
without your patch:

  error: ‘warn_if_not_aligned’ may not be specified for ‘foo2’


Please try it with Linux/alpha cross compiler.


I get the same error (both with and without your patch). Do
I need to be using some specific options?  (-Wif-not-aligned
makes no difference.)

I'm also not sure I understand how the target affects this
case (is there supposed to be some other attribute on the
declaration above? attribute aligned).  I thought the
handling of function declarations with the warn_if_not_aligned
attribute was target independent.

In any case, I think it would be easier if the patch included
a test showing what the problem is and how the change prevents
it.

Martin

Re: [PATCH 1/2] Don't warn function alignment if warn_if_not_aligned_p is true

[H.J. Lu]

On Fri, Aug 25, 2017 at 1:14 PM, Martin Sebor  wrote:
> On 08/25/2017 01:35 PM, H.J. Lu wrote:
>>
>> On Fri, Aug 25, 2017 at 12:24 PM, Martin Sebor  wrote:
>>>
>>> On 08/25/2017 11:31 AM, H.J. Lu wrote:


 On Fri, Aug 25, 2017 at 10:15 AM, Martin Sebor  wrote:
>
>
> On 08/21/2017 06:21 AM, H.J. Lu wrote:
>>
>>
>>
>> When warn_if_not_aligned_p is true, a warning will be issued on
>> function
>> declaration later.  There is no need to warn function alignment when
>> warn_if_not_aligned_p is true.
>>
>> OK for trunk?
>>
>> H.J.
>> --
>> * c-attribs.c (common_handle_aligned_attribute): Don't warn
>> function alignment if warn_if_not_aligned_p is true.
>> ---
>>  gcc/c-family/c-attribs.c | 5 -
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
>> index 5f79468407f..78969532543 100644
>> --- a/gcc/c-family/c-attribs.c
>> +++ b/gcc/c-family/c-attribs.c
>> @@ -1754,9 +1754,12 @@ common_handle_aligned_attribute (tree *node,
>> tree
>> args, int flags,
>>This formally comes from the c++11 specification but we are
>>doing it for the GNU attribute syntax as well.  */
>>  *no_add_attrs = true;
>> -  else if (TREE_CODE (decl) == FUNCTION_DECL
>> +  else if (!warn_if_not_aligned_p
>> +  && TREE_CODE (decl) == FUNCTION_DECL
>>&& DECL_ALIGN (decl) > (1U << i) * BITS_PER_UNIT)
>>  {
>> +  /* Don't warn function alignment here if warn_if_not_aligned_p
>> is
>> +true.  It will be warned later.  */
>>if (DECL_USER_ALIGN (decl))
>> error ("alignment for %q+D was previously specified as %d "
>>"and may not be decreased", decl,
>
>
>
>
> Your comment refers to warning but the code here uses error().
> That raises two questions for me: a) will the later diagnostic
> really be a warning or an error, and if a warning, under what
> option will it be issued? and b) why is an error appropriate
> here when a warning is appropriate elsewhere (most other
> attribute conflicts are at present diagnosed with -Wattributes).



 It can be changed to warning.
>>>
>>>
>>>
>>> Okay, that's goo to hear (it validates what I did in the patch
>>> I referenced).
>>>
>>> As for your patch, I don't quite see ho the block can ever be
>>> entered.  In fact, I had to make changes to the function in
>>> my patch below to let it detect and diagnose the condition
>>> (attempting to reduce the alignment of a function).  The
>>> details are in bug 81566.  Did I miss something?
>>
>>
>> Try this:
>>
>> __attribute__((warn_if_not_aligned(8)))
>> void
>> foo2 (void)
>> {
>> }
>
>
> For me the top of trunk prints the same error both with and
> without your patch:
>
>   error: ‘warn_if_not_aligned’ may not be specified for ‘foo2’

Please try it with Linux/alpha cross compiler.

> The problem I was referring to above (bug 81566), and the reason
> why I said the block in the if statement modified by in your patch
> is unreachable is that its controlling expression is a subset of
> the test above it.  See comment 2 in bug 81566.
>
> Martin
>



-- 
H.J.

Re: 0006-Part-6.-Add-x86-tests-for-Intel-CET-implementation

[Jeff Law]

On 08/01/2017 02:56 AM, Tsimbalist, Igor V wrote:
> Part#6. Add x86 tests for Intel CET implementation.
> 
> 
> 0006-Part-6.-Add-x86-tests-for-Intel-CET-implementation.patch
> 
> 
> From e4a8227e83e8e9f3ddbaa97707f3d335009e0e77 Mon Sep 17 00:00:00 2001
> From: Igor Tsimbalist 
> Date: Fri, 21 Jul 2017 19:40:40 +0300
> Subject: [PATCH 6/9] Part#6. Add x86 tests for Intel CET implementation.
> 
> gcc/testsuite/
> 
>   * g++.dg/cet-notrack-1.C: New test.
>   * gcc.target/i386/cet-intrin-1.c: Likewise.
>   * gcc.target/i386/cet-intrin-10.c: Likewise.
>   * gcc.target/i386/cet-intrin-2.c: Likewise.
>   * gcc.target/i386/cet-intrin-3.c: Likewise.
>   * gcc.target/i386/cet-intrin-4.c: Likewise.
>   * gcc.target/i386/cet-intrin-5.c: Likewise.
>   * gcc.target/i386/cet-intrin-6.c: Likewise.
>   * gcc.target/i386/cet-intrin-7.c: Likewise.
>   * gcc.target/i386/cet-intrin-8.c: Likewise.
>   * gcc.target/i386/cet-intrin-9.c: Likewise.
>   * gcc.target/i386/cet-label.c: Likewise.
>   * gcc.target/i386/cet-notrack-1a.c: Likewise.
>   * gcc.target/i386/cet-notrack-1b.c: Likewise.
>   * gcc.target/i386/cet-notrack-2a.c: Likewise.
>   * gcc.target/i386/cet-notrack-2b.c: Likewise.
>   * gcc.target/i386/cet-notrack-3.c: Likewise.
>   * gcc.target/i386/cet-notrack-4a.c: Likewise.
>   * gcc.target/i386/cet-notrack-4b.c: Likewise.
>   * gcc.target/i386/cet-notrack-5a.c: Likewise.
>   * gcc.target/i386/cet-notrack-5b.c: Likewise.
>   * gcc.target/i386/cet-notrack-6a.c: Likewise.
>   * gcc.target/i386/cet-notrack-6b.c: Likewise.
>   * gcc.target/i386/cet-notrack-7.c: Likewise.
>   * gcc.target/i386/cet-property-1.c: Likewise.
>   * gcc.target/i386/cet-property-2.c: Likewise.
>   * gcc.target/i386/cet-rdssp-1.c: Likewise.
>   * gcc.target/i386/cet-sjlj-1.c: Likewise.
>   * gcc.target/i386/cet-sjlj-2.c: Likewise.
>   * gcc.target/i386/cet-sjlj-3.c: Likewise.
>   * gcc.target/i386/cet-switch-1.c: Likewise.
>   * gcc.target/i386/cet-switch-2.c: Likewise.
>   * lib/target-supports.exp (check_effective_target_cet): New
>   proc.
Whoops.  NEvermind my previous comment about x86 specific tests.  I
should have scanned the whole kit before starting to comment on the
earlier patches.

Uros will have the say on the x86 specific bits.  Given it's been 3
weeks, you might want to ping him directly to start getting his feedback.

jeff

Re: 0003-Part-3.-Add-tests-for-finstrument-control-flow-and-notrack attribute

[Jeff Law]

On 08/01/2017 02:56 AM, Tsimbalist, Igor V wrote:
> Part#3. Add tests for -finstrument-control-flow and notrack attribute.
> 
> 
> 0003-Part-3.-Add-tests-for-finstrument-control-flow-and-n.patch
> 
> 
> From 7869de8a0c0ec55c4e9240c2483fefee97bf34c9 Mon Sep 17 00:00:00 2001
> From: Igor Tsimbalist 
> Date: Mon, 3 Jul 2017 17:29:08 +0300
> Subject: [PATCH 3/9] Part#3. Add tests for -finstrument-control-flow and
>  notrack attribute.
> 
> gcc/testsuite/
> 
>   * c-c++-common/finstrument-control-flow.c: New test.
>   * c-c++-common/notrack-1.c: Likewise.
>   * c-c++-common/notrack-2.c: Likewise.
No concerns with the existing tests.

We should consider an ICF test as I outlined in an earlier message.

We should also consider tests where we drop/add the notrack attribute as
ISTM we ought to be getting warnings in those cases.

Finally, you should consider tests in gcc.target/i386 that verify we
generate the proper instrumentation for a  few tests.

jeff

Re: 0002-Part-2.-Document-finstrument-control-flow-and-notrack attribute

[Jeff Law]

On 08/01/2017 02:56 AM, Tsimbalist, Igor V wrote:
> Part#2. Document -finstrument-control-flow and notrack attribute.
> 
> 
> 0002-Part-2.-Document-finstrument-control-flow-and-notrac.patch
> 
> 
> From c3e45c80731672e74d638f787e80ba975279b9b9 Mon Sep 17 00:00:00 2001
> From: Igor Tsimbalist 
> Date: Mon, 3 Jul 2017 17:12:49 +0300
> Subject: [PATCH 2/9] Part#2. Document -finstrument-control-flow and notrack
>  attribute.
> 
> gcc/
>   * doc/extend.texi: Add 'notrack' documentation.
>   * doc/invoke.texi: Add -finstrument-control-flow documentation.
>   * doc/rtl.texi: Add REG_CALL_NOTRACK documenation.
> ---
>  gcc/doc/extend.texi | 52 
>  gcc/doc/invoke.texi | 22 ++
>  gcc/doc/rtl.texi| 15 +++
>  3 files changed, 89 insertions(+)
> 
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index 6934b4c..80de8a7 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -5632,6 +5632,58 @@ Specify which floating-point unit to use.  You must 
> specify the
>  @code{target("fpmath=sse,387")} option as
>  @code{target("fpmath=sse+387")} because the comma would separate
>  different options.
> +
> +@item notrack
> +@cindex @code{notrack} function attribute
> +The @code{notrack} attribute on a function is used to inform the
> +compiler that the function's prolog should not be instrumented when
> +compiled with the @option{-finstrument-control-flow} option.  The
> +compiler assumes that the function's address is a valid target for a
> +control-flow transfer.
Is the default to instrument everything when -finstrument-control-flow
is enabled?  Or can we avoid instrumentation on a function that never
has its address taken (ie, it is only called via a call instruction?)

> +
> +The @code{notrack} attribute on a type of pointer to function is
> +used to inform the compiler that a call through the pointer should
> +not be instrumented when compiled with the
> +@option{-finstrument-control-flow} option.  The compiler assumes
> +that the function's address from the pointer is a valid target for
> +a control-flow transfer.  A direct function call through a function
> +name is assumed as a save call thus direct calls will not be
> +instrumented by the compiler.
s/save/safe/

FWIW, I think putting the attribute into in the type system is a good
thing :-)

> +
> +The @code{notrack} attribute is applied to an object's type.  A
> +The @code{notrack} attribute is transfered to a call instruction at
> +the GIMPLE and RTL translation phases.  The attribute is not propagated
> +through assignment, store and load.
> +
> +@smallexample
> +@{
> +void (*foo)(void) __attribute__(notrack);
> +void (*foo1)(void) __attribute__(notrack);
> +void (*foo2)(void);
> +
> +int
> +foo (void) /* The function's address is not tracked.  */
> +
> +  /* This call site is not tracked for
> + control-flow instrumentation.  */
> +  (*foo1)();
> +  foo1 = foo2;
> +  /* This call site is still not tracked for
> + control-flow instrumentation.  */
> +  (*foo1)();
> +
> +  /* This call site is tracked for
> + control-flow instrumentation.  */
> +  (*foo2)();
> +  foo2 = foo1;
> +  /* This call site is still tracked for
> + control-flow instrumentation.  */
> +  (*foo2)();
> +
> +  return 0;
> +@}
> +@end smallexample
Given the notrack attribute is part of the type system, could we issue a
warning on the foo1 = foo2 assignment since we're discarding tracking
that's implicit on foo2?

> +
>  @end table
>  
>  On the x86, the inliner does not inline a
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 5ae9dc4..ff2ce92 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -459,6 +459,7 @@ Objective-C and Objective-C++ Dialects}.
>  -fchkp-check-read  -fchkp-check-write  -fchkp-store-bounds @gol
>  -fchkp-instrument-calls  -fchkp-instrument-marked-only @gol
>  -fchkp-use-wrappers  -fchkp-flexible-struct-trailing-arrays@gol
> +-finstrument-control-flow @gol
>  -fstack-protector  -fstack-protector-all  -fstack-protector-strong @gol
>  -fstack-protector-explicit  -fstack-check @gol
>  -fstack-limit-register=@var{reg}  -fstack-limit-symbol=@var{sym} @gol
> @@ -11284,6 +11285,27 @@ is used to link a program, the GCC driver 
> automatically links
>  against @file{libmpxwrappers}.  See also @option{-static-libmpxwrappers}.
>  Enabled by default.
>  
> +@item -finstrument-control-flow
> +@opindex finstrument-control-flow
> +@opindex fno-instrument-control-flow
> +Enable code instrumentation of control-flow transfers to increase
> +a program security by checking a target address of control-flow
> +transfer instructions (i.e. routine call, routine return, jump)
> +are valid targets.  This prevents diverting the control flow
> +instructions from its original target address to a new undesigned
> +target.  This is intended to protect against such theats as
> +Return-oriented Programming (ROP), 

Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling

[Jeff Law]

On 08/01/2017 02:56 AM, Tsimbalist, Igor V wrote:
> Part#1. Add generic part for Intel CET enabling.
> 
> The spec is available at
> 
> https://software.intel.com/sites/default/files/managed/4d/2a/control-flow-enforcement-technology-preview.pdf
> 
> High-level design.
> --
> 
> A proposal is to introduce a target independent flag
> -finstrument-control-flow with a semantic to instrument a code to
> control validness or integrity of control-flow transfers using jump
> and call instructions. The main goal is to detect and block a possible
> malware execution through transfer the execution to unknown target
> address. Implementation could be either software or target based. Any
> target platforms can provide their implementation for instrumentation
> under this option.
> 
> When the -finstrument-control-flow flag is set each implementation has
> to check if a support exists for a target platform and report an error
> if no support is found.
> 
> The compiler should instrument any control-flow transfer points in a
> program (ex. call/jmp/ret) as well as any landing pads, which are
> targets of for control-flow transfers.
> 
> A new 'notrack' attribute is introduced to provide hand tuning support.
> The attribute directs the compiler to skip a call to a function and a
> function's landing pad from instrumentation (tracking). The attribute
> can be used for function and pointer to function types, otherwise it
> will be ignored. The attribute is saved in a type and propagated to a
> GIMPLE call statement and later to a call instruction.
> 
> Currently all platforms except i386 will report the error and do no
> instrumentation. i386 will provide the implementation based on a
> specification published by Intel for a new technology called
> Control-flow Enforcement Technology (CET).
> 
> 
> 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling.patch
> 
> 
> From 403fc8239fb1f690cc378287b4def57dcc9d25bf Mon Sep 17 00:00:00 2001
> From: Igor Tsimbalist 
> Date: Mon, 3 Jul 2017 17:11:58 +0300
> Subject: [PATCH 1/9] Part#1. Add generic part for Intel CET enabling.
> 
> The spec is available at
> 
> https://software.intel.com/sites/default/files/managed/4d/2a/control-flow-enforcement-technology-preview.pdf
> 
> High-level design.
> --
> 
> A proposal is to introduce a target independent flag
> -finstrument-control-flow with a semantic to instrument a code to
> control validness or integrity of control-flow transfers using jump
> and call instructions. The main goal is to detect and block a possible
> malware execution through transfer the execution to unknown target
> address. Implementation could be either software or target based. Any
> target platforms can provide their implementation for instrumentation
> under this option.
> 
> When the -finstrument-control-flow flag is set each implementation has
> to check if a support exists for a target platform and report an error
> if no support is found.
> 
> The compiler should instrument any control-flow transfer points in a
> program (ex. call/jmp/ret) as well as any landing pads, which are
> targets of for control-flow transfers.
> 
> A new 'notrack' attribute is introduced to provide hand tuning support.
> The attribute directs the compiler to skip a call to a function and a
> function's landing pad from instrumentation (tracking). The attribute
> can be used for function and pointer to function types, otherwise it
> will be ignored. The attribute is saved in a type and propagated to a
> GIMPLE call statement and later to a call instruction.
> 
> Currently all platforms except i386 will report the error and do no
> instrumentation. i386 will provide the implementation based on a
> specification published by Intel for a new technology called
> Control-flow Enforcement Technology (CET).
> 
> gcc/c-family/
> 
>   * c-attribs.c (handle_notrack_attribute): New function.
>   (c_common_attribute_table): Add 'notrack' handling.
> 
> gcc/
> 
>   * cfgexpand.c (expand_call_stmt): Set REG_CALL_NOTRACK.
>   * combine.c (distribute_notes): Add REG_CALL_NOTRACK handling.
>   * common.opt: Add finstrument-control-flow flag.
>   * emit-rtl.c (try_split): Add REG_CALL_NOTRACK handling.
>   * gimple.c (gimple_build_call_from_tree): Add 'notrack'
>   propogation.
>   * gimple.h (gf_mask): Add GF_CALL_WITH_NOTRACK.
>   (gimple_call_with_notrack_p): function.
>   (gimple_call_set_with_notrack): Likewise.
>   * recog.c (peep2_attempt): Add REG_CALL_NOTRACK handling.
>   * reg-notes.def: Add REG_NOTE (CALL_NOTRACK).
>   * toplev.c (process_options): Add flag_instrument_control_flow
>   handling.
> ---
>  gcc/c-family/c-attribs.c | 23 +++
>  gcc/cfgexpand.c  | 11 +++
>  gcc/combine.c|  1 +
>  gcc/common.opt   |  5 +
>  gcc/emit-rtl.c   |  1 +
>  gcc/gimple.c | 17 +
>  gcc/gimple.h  

Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling

[Jeff Law]

On 08/15/2017 07:42 AM, Richard Biener wrote:
> 
> Please change the names to omit 'with_', thus just notrack
> and GF_CALL_NOTRACK.
> 
> I think 'notrack' is somewhat unspecific of a name, what prevented
> you to use 'nocet'?
I think we should look for something better than notrack.  I think
"control flow enforcement/CFE" is commonly used for this stuff.  CET is
an Intel marketing name IIRC.

The tracking is for indirect branch/call targets.  So some combination
of cfe, branch/call and track should be sufficient.



> Any idea how to implement a software-based solution efficiently?
> Creating a table of valid destination addresses in a special section
> should be possible without too much work, am I right in that only
> indirect control transfer is checked?  Thus CET assumes the
> code itself cannot be changed (and thus the stack isn't executable)?
Well, there's two broad areas that have to be addressed.

First you need to separate the call stack from the rest of the call
frame, or at least the parts of the call frame that are potentially
vulnerable to overruns.  LLVM has some code to do this.  Essentially any
object in the stack that is not proven to be safely accessed gets put
into a separate stack.  That roughly duplicates the shadow stack
capability.  I think their implementation is just x86 and IIRC doesn't
work in some circumstances -- I'd consider it a proof of concept, not
something ready for production use.


Bernd and I also spec'd a couple more approaches to protect the return
address.  Essentially, the return address turns into a cookie that a
particular caller can use to lookup/map to a real return address.  We
didn't take any of this to completion because it was pretty clear the
ROP mitigation landscape was going to change and make software only
solutions less appealing.

Second you need the indirect branch/call tracking.  I spec'd something
out in this space with Intel's engineers years ago.  Essentially
building tables of valid targets for indirect branches and checking
instrumentation.   You can have a global table, per-DSO tables or you
can have a per-branch table.  It gets a little hairy in mixed mode
environments.  But it basically works how you'd expect.  Indirect
branches/calls turn into something considerably more complex as do the
branch/call targets if you have access to something like a
last-taken-branch.

Jeff

Re: [PATCH 1/2] Don't warn function alignment if warn_if_not_aligned_p is true

[Martin Sebor]

On 08/25/2017 01:35 PM, H.J. Lu wrote:

On Fri, Aug 25, 2017 at 12:24 PM, Martin Sebor  wrote:

On 08/25/2017 11:31 AM, H.J. Lu wrote:


On Fri, Aug 25, 2017 at 10:15 AM, Martin Sebor  wrote:


On 08/21/2017 06:21 AM, H.J. Lu wrote:



When warn_if_not_aligned_p is true, a warning will be issued on function
declaration later.  There is no need to warn function alignment when
warn_if_not_aligned_p is true.

OK for trunk?

H.J.
--
* c-attribs.c (common_handle_aligned_attribute): Don't warn
function alignment if warn_if_not_aligned_p is true.
---
 gcc/c-family/c-attribs.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 5f79468407f..78969532543 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -1754,9 +1754,12 @@ common_handle_aligned_attribute (tree *node, tree
args, int flags,
   This formally comes from the c++11 specification but we are
   doing it for the GNU attribute syntax as well.  */
 *no_add_attrs = true;
-  else if (TREE_CODE (decl) == FUNCTION_DECL
+  else if (!warn_if_not_aligned_p
+  && TREE_CODE (decl) == FUNCTION_DECL
   && DECL_ALIGN (decl) > (1U << i) * BITS_PER_UNIT)
 {
+  /* Don't warn function alignment here if warn_if_not_aligned_p is
+true.  It will be warned later.  */
   if (DECL_USER_ALIGN (decl))
error ("alignment for %q+D was previously specified as %d "
   "and may not be decreased", decl,




Your comment refers to warning but the code here uses error().
That raises two questions for me: a) will the later diagnostic
really be a warning or an error, and if a warning, under what
option will it be issued? and b) why is an error appropriate
here when a warning is appropriate elsewhere (most other
attribute conflicts are at present diagnosed with -Wattributes).



It can be changed to warning.



Okay, that's goo to hear (it validates what I did in the patch
I referenced).

As for your patch, I don't quite see ho the block can ever be
entered.  In fact, I had to make changes to the function in
my patch below to let it detect and diagnose the condition
(attempting to reduce the alignment of a function).  The
details are in bug 81566.  Did I miss something?


Try this:

__attribute__((warn_if_not_aligned(8)))
void
foo2 (void)
{
}


For me the top of trunk prints the same error both with and
without your patch:

  error: ‘warn_if_not_aligned’ may not be specified for ‘foo2’

The problem I was referring to above (bug 81566), and the reason
why I said the block in the if statement modified by in your patch
is unreachable is that its controlling expression is a subset of
the test above it.  See comment 2 in bug 81566.

Martin

Re: [PATCH] istream_iterator: unexpected read in ctor

[Tim Song]

On Thu, Aug 24, 2017 at 4:55 AM, Petr Ovtchenkov  wrote:
> istream_iterator do unexpected read from stream
> when initialized by istream&.
>

This is pretty much required by the specification. See the discussion
in http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0738r0.html

And the "fix" is also wrong because it makes operator* not
thread-safe, which it is required to be (as a const member function).

Re: [RFC] [PATCH] Introduce configure flag --with-stage1-cflags.

[Jeff Law]

On 07/31/2017 01:47 AM, Martin Liška wrote:
> I would like to ping this. Input from other people will be appreciated ;)
I think the thing to keep in mind here is that IIUC this only affects
things when we've configured using the --with-stage1-cflags option.

So questions about is -O1 more stable than -O2, should we restrict -O2
to newer compilers, etc are really more about the defaults we set.

My understanding is the patch is just adding the capability and does not
change the default.  Assuming that's the case, then I'm comfortable
acking the raw infrastructure.

jeff

Re: [PATCH 1/2] Don't warn function alignment if warn_if_not_aligned_p is true

[H.J. Lu]

On Fri, Aug 25, 2017 at 12:24 PM, Martin Sebor  wrote:
> On 08/25/2017 11:31 AM, H.J. Lu wrote:
>>
>> On Fri, Aug 25, 2017 at 10:15 AM, Martin Sebor  wrote:
>>>
>>> On 08/21/2017 06:21 AM, H.J. Lu wrote:


 When warn_if_not_aligned_p is true, a warning will be issued on function
 declaration later.  There is no need to warn function alignment when
 warn_if_not_aligned_p is true.

 OK for trunk?

 H.J.
 --
 * c-attribs.c (common_handle_aligned_attribute): Don't warn
 function alignment if warn_if_not_aligned_p is true.
 ---
  gcc/c-family/c-attribs.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

 diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
 index 5f79468407f..78969532543 100644
 --- a/gcc/c-family/c-attribs.c
 +++ b/gcc/c-family/c-attribs.c
 @@ -1754,9 +1754,12 @@ common_handle_aligned_attribute (tree *node, tree
 args, int flags,
This formally comes from the c++11 specification but we are
doing it for the GNU attribute syntax as well.  */
  *no_add_attrs = true;
 -  else if (TREE_CODE (decl) == FUNCTION_DECL
 +  else if (!warn_if_not_aligned_p
 +  && TREE_CODE (decl) == FUNCTION_DECL
&& DECL_ALIGN (decl) > (1U << i) * BITS_PER_UNIT)
  {
 +  /* Don't warn function alignment here if warn_if_not_aligned_p is
 +true.  It will be warned later.  */
if (DECL_USER_ALIGN (decl))
 error ("alignment for %q+D was previously specified as %d "
"and may not be decreased", decl,
>>>
>>>
>>>
>>> Your comment refers to warning but the code here uses error().
>>> That raises two questions for me: a) will the later diagnostic
>>> really be a warning or an error, and if a warning, under what
>>> option will it be issued? and b) why is an error appropriate
>>> here when a warning is appropriate elsewhere (most other
>>> attribute conflicts are at present diagnosed with -Wattributes).
>>
>>
>> It can be changed to warning.
>
>
> Okay, that's goo to hear (it validates what I did in the patch
> I referenced).
>
> As for your patch, I don't quite see ho the block can ever be
> entered.  In fact, I had to make changes to the function in
> my patch below to let it detect and diagnose the condition
> (attempting to reduce the alignment of a function).  The
> details are in bug 81566.  Did I miss something?

Try this:

__attribute__((warn_if_not_aligned(8)))
void
foo2 (void)
{
}


-- 
H.J.

Re: [PATCH 1/2] Don't warn function alignment if warn_if_not_aligned_p is true

[Martin Sebor]

On 08/25/2017 11:31 AM, H.J. Lu wrote:

On Fri, Aug 25, 2017 at 10:15 AM, Martin Sebor  wrote:

On 08/21/2017 06:21 AM, H.J. Lu wrote:


When warn_if_not_aligned_p is true, a warning will be issued on function
declaration later.  There is no need to warn function alignment when
warn_if_not_aligned_p is true.

OK for trunk?

H.J.
--
* c-attribs.c (common_handle_aligned_attribute): Don't warn
function alignment if warn_if_not_aligned_p is true.
---
 gcc/c-family/c-attribs.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 5f79468407f..78969532543 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -1754,9 +1754,12 @@ common_handle_aligned_attribute (tree *node, tree
args, int flags,
   This formally comes from the c++11 specification but we are
   doing it for the GNU attribute syntax as well.  */
 *no_add_attrs = true;
-  else if (TREE_CODE (decl) == FUNCTION_DECL
+  else if (!warn_if_not_aligned_p
+  && TREE_CODE (decl) == FUNCTION_DECL
   && DECL_ALIGN (decl) > (1U << i) * BITS_PER_UNIT)
 {
+  /* Don't warn function alignment here if warn_if_not_aligned_p is
+true.  It will be warned later.  */
   if (DECL_USER_ALIGN (decl))
error ("alignment for %q+D was previously specified as %d "
   "and may not be decreased", decl,



Your comment refers to warning but the code here uses error().
That raises two questions for me: a) will the later diagnostic
really be a warning or an error, and if a warning, under what
option will it be issued? and b) why is an error appropriate
here when a warning is appropriate elsewhere (most other
attribute conflicts are at present diagnosed with -Wattributes).


It can be changed to warning.


Okay, that's goo to hear (it validates what I did in the patch
I referenced).

As for your patch, I don't quite see ho the block can ever be
entered.  In fact, I had to make changes to the function in
my patch below to let it detect and diagnose the condition
(attempting to reduce the alignment of a function).  The
details are in bug 81566.  Did I miss something?

Martin




My main motivation for these questions is to understand the
rationale for warning for vs rejecting conflicts so that
a consistent general solution can be implemented for all
attributes (i.e., along the lines of my patch here:
https://gcc.gnu.org/ml/gcc-patches/2017-08/msg01457.html)

Martin

Re: [PATCHv2][PING][PR 59521] Respect __builtin_expect in switch statements

[Jeff Law]

On 07/27/2017 09:21 PM, Yury Gribov wrote:
> Hi all,
> 
> This is a ping for
> https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01275.html . I've fixed
> attachment type, hopefully it's easier to read now.
> 
> This patch adds support for __builtin_expect in switch statements at
> tree level (RTL part would be reviewed/commited separately).  It's an
> update of https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01016.html ,
> rebased and retested.
> 
> Ok for trunk?
> 
> -Y
> 
> pr59521-2.patch
> 
> 
> 2017-07-21  Yury Gribov  
>   Martin Liska  
> 
>   PR middle-end/59521
> 
> gcc/
>   * predict.c (set_even_probabilities): Handle case of a single
>   likely edge.
>   (combine_predictions_for_bb): Ditto.
>   (tree_predict_by_opcode): Handle switch statements.
>   * stmt.c (balance_case_nodes): Select pivot value based on
>   probabilities.
> 
> gcc/testsuite/
>   * gcc.dg/predict-15.c: New test.
Your patch includes changes to tree-cfg.c.  Those need to be mentioned
in the ChangeLog.

Your ChangeLog entry mentions a stmt.c change, but there is no such
change in the patch itself.

The bits that are actually in the patch are fine.  You just need to
verify they're what you wanted to submit, and if so make sure they have
a correct ChangeLog and they're OK.

jeff

Re: [PATCH][GCC][AARCH64]Bad code-gen for structure/block/unaligned memory access

[H.J. Lu]

On Sun, Jul 2, 2017 at 11:17 PM, Tamar Christina
 wrote:
> Hi All,
>
> Sorry I just realized I never actually sent the updated patch...
>
> So here it is :)
>

Are there any testcases?

-- 
H.J.

Re: [PATCH][GCC][AARCH64]Bad code-gen for structure/block/unaligned memory access

[Jeff Law]

On 07/03/2017 12:17 AM, Tamar Christina wrote:
> Hi All,
> 
> Sorry I just realized I never actually sent the updated patch...
AFAICT this never got resolved.

I'll let the aarch64 folks own those changes.  I'm going to focus just
on the small change to expr.c where you change the computation of
bitsize in copy_blkmode_to_reg.

My biggest worry is that this could end up breaking STRICT_ALIGNMENT
targets.  Thankfully, the default definition of SLOW_UNALIGNED_ACCESS is
STRICT_ALIGNMENT.  So on a STRICT_ALIGNMENT target that does not define
SLOW_UNALIGNED_ACCESS we should OK.  I also did a quick scan of
STRICT_ALIGNMENT targets that override SLOW_UNALIGNED_ACCESS and they
seem reasonable.

So I'll ack the expr.c change.  You'll need to chase down an ack on the
aarch64 specific changes.


jeff

> 
> So here it is :)
> 
> Regards,
> Tamar
> 
> From: gcc-patches-ow...@gcc.gnu.org  on behalf 
> of Tamar Christina 
> Sent: Friday, June 9, 2017 4:51:52 PM
> To: Richard Sandiford
> Cc: GCC Patches; nd; James Greenhalgh; Richard Earnshaw; Marcus Shawcroft
> Subject: RE: [PATCH][GCC][AARCH64]Bad code-gen for structure/block/unaligned 
> memory access
> 
>>> +  /* Optimize routines for MEM to REG copies.  */  if (n < 8 &&
>>> + !REG_P (XEXP (operands[0], 0)))
>>
>> This seems to be checking that the address of the original destination
>> memory isn't a plain base register.  Why's it important to reject that case 
>> but
>> allow e.g. base+offset?
> 
> this function is (as far as I could tell) only being called with two types of 
> destinations:
> a location on the stack or a plain register.
> 
> When the destination is a register such as with
> 
> void Fun3(struct struct3 foo3)
> {
>   L3 = foo3;
> }
> 
> You run into the issue you had pointed to below where we might write too 
> much. Ideally the constraint
> I would like is to check if the destination is either a new register (no 
> data) or that the structure was padded.
> 
> I couldn't figure out how to do this and the gains over the existing code for 
> this case was quite small.
> So I just disallowed it leaving it to the existing code, which isn't so bad, 
> only 1 extra instruction.
> 
>>
>>> +   {
>>> + unsigned int max_align = UINTVAL (operands[2]);
>>> + max_align = n < max_align ? max_align : n;
>>
>> Might be misunderstanding, but isn't max_align always equal to n here, since
>> n was set by:
> 
> Correct, previously this patch was made to allow n < 16. These were left over 
> from the cleanup.
> I'll correct.
> 
>>
>>> + result = gen_rtx_IOR (dest_mode, reg, result);
>>> +  }
>>> +
>>> +dst = adjust_address (dst, dest_mode, 0);
>>> +emit_insn (gen_move_insn (dst, result));
>>
>> dest_mode was chosen by smallest_mode_for_size, so can be bigger than n.
>> Doesn't that mean that we'll write beyond the end of the copy region when
>> n is an awkward number?
> 
> Yes, see my answer above. For the other case, when we write onto a location 
> on the stack, this is fine
> due to the alignment.
> 
>>
>>> diff --git a/gcc/expr.c b/gcc/expr.c
>>> index
>>>
>> 91d7ea217229fac62380b5d4b646961bf7c836c1..b1df4651e7942346007cda1cce
>> 8e
>>> e5a19297ab16 100644
>>> --- a/gcc/expr.c
>>> +++ b/gcc/expr.c
>>> @@ -2743,7 +2743,9 @@ copy_blkmode_to_reg (machine_mode mode,
>> tree
>>> src)
>>>
>>>n_regs = (bytes + UNITS_PER_WORD - 1) / UNITS_PER_WORD;
>>>dst_words = XALLOCAVEC (rtx, n_regs);
>>> -  bitsize = MIN (TYPE_ALIGN (TREE_TYPE (src)), BITS_PER_WORD);
>>> +  bitsize = BITS_PER_WORD;
>>> +  if (SLOW_UNALIGNED_ACCESS (BLKmode, TYPE_ALIGN (TREE_TYPE
>> (src
>>> +bitsize = MIN (TYPE_ALIGN (TREE_TYPE (src)), BITS_PER_WORD);
>>
>> I think this ought to be testing word_mode instead of BLKmode.
>> (Testing BLKmode doesn't really make sense in general, because the mode
>> doesn't have a meaningful alignment.)
> 
> Ah, yes that makes sense. I'll update the patch.
> 
> New patch is validating and will submit it soon.
> 
>>
>> Thanks,
>> Richard

[PATCH] Fix fortran tests to not print out non-ascii characters

[Steve Ellcey]

This is a minor patch for a minor problem, I will check it in as
obvious if no one objects.  While looking at another issue I found that
the gfortran.log file created by testing contains some non-ascii
characters, this bothered me and caused some problems when grepping for
things.  I tracked down the problem to these 3 tests that are printing
out 0 valued character variables.  The variables are supposed to be 0,
because that is what the test is checking for, so my fix is to print
out the ichar value of the character variable instead of the variable
itself.

Steve Ellcey
sell...@cavium.com


2017-08-25  Steve Ellcey  

* gfortran.dg/dec_init_1.f90 (dummy): Use ichar in print statement.
* gfortran.dg/dec_init_2.f90 (dummy): Ditto.
* gfortran.dg/dec_init_3.f90 (dummy): Ditto.

diff --git a/gcc/testsuite/gfortran.dg/dec_init_1.f90 b/gcc/testsuite/gfortran.dg/dec_init_1.f90
index 03ada9c..5e1956a 100644
--- a/gcc/testsuite/gfortran.dg/dec_init_1.f90
+++ b/gcc/testsuite/gfortran.dg/dec_init_1.f90
@@ -14,7 +14,7 @@ subroutine dummy(i1,r1,c1,l1,i2,r2,c2,l2)
   real, intent(inout) :: r2
   character, intent(inout) :: c2
   logical, intent(inout) :: l2
-  print *, i1, i2, l1, l2, c1, c2, r1, r2
+  print *, i1, i2, l1, l2, ichar(c1), ichar(c2), r1, r2
   if ( i1 .ne. 0 .or. i2 .ne. 0 ) call abort()
   if ( l1 .or. l2 ) call abort()
   if ( c1 .ne. achar(0) .or. c2 .ne. achar(0) ) call abort()
diff --git a/gcc/testsuite/gfortran.dg/dec_init_2.f90 b/gcc/testsuite/gfortran.dg/dec_init_2.f90
index 41deac9..eae5549 100644
--- a/gcc/testsuite/gfortran.dg/dec_init_2.f90
+++ b/gcc/testsuite/gfortran.dg/dec_init_2.f90
@@ -15,7 +15,7 @@ subroutine dummy(i1,r1,c1,l1,i2,r2,c2,l2)
   real, intent(inout) :: r2
   character, intent(inout) :: c2
   logical, intent(inout) :: l2
-  print *, i1, i2, l1, l2, c1, c2, r1, r2
+  print *, i1, i2, l1, l2, ichar(c1), ichar(c2), r1, r2
   if ( i1 .ne. 42 .or. i2 .ne. 42 ) call abort()
   if ( (.not. l1) .or. (.not. l2) ) call abort()
   if ( c1 .ne. achar(32) .or. c2 .ne. achar(32) ) call abort()
diff --git a/gcc/testsuite/gfortran.dg/dec_init_3.f90 b/gcc/testsuite/gfortran.dg/dec_init_3.f90
index 6c1161a..253cd9b 100644
--- a/gcc/testsuite/gfortran.dg/dec_init_3.f90
+++ b/gcc/testsuite/gfortran.dg/dec_init_3.f90
@@ -14,7 +14,7 @@ subroutine dummy(i1,r1,c1,l1,i2,r2,c2,l2)
   real, intent(inout) :: r2
   character, intent(inout) :: c2
   logical, intent(inout) :: l2
-  print *, i1, i2, l1, l2, c1, c2, r1, r2
+  print *, i1, i2, l1, l2, ichar(c1), ichar(c2), r1, r2
   if ( i1 .ne. 0 .or. i2 .ne. 0 ) call abort()
   if ( l1 .or. l2 ) call abort()
   if ( c1 .ne. achar(0) .or. c2 .ne. achar(0) ) call abort()

Re: Statement Frontier Notes, Location Views, and Inlined Entry Point Markers

[Alexandre Oliva]

On Aug 23, 2017, Richard Biener  wrote:

> Just curious if, for example, --with-build-config=bootstrap-lto
> --enable-languages=c
> --disable-multilib and make -j1 shows any difference in time and/or peak 
> memory
> use (the interesting peak memory use is that of the WPA phase).

$ ../configure --enable-languages=c --disable-multilib \
  --with-build-config=bootstrap-lto -C
$ make -jN
$ cd gcc
$ make clean
$ /bin/time make -j1
3860.82user 92.39system 1:06:12elapsed 99%CPU (0avgtext+0avgdata 
952460maxresident)k
0inputs+2338008outputs (0major+53550802minor)pagefaults 0swaps

$ make clean
$ /usr/bin/time make -j1 \
  C{,XX}FLAGS='-g -gno-statement-frontiers -O2 -flto=jobserver -frandom-seed=1'
-> error, lto fails to drop inline entry stmts from libiberty.a; will fix, 
meanwhile:

$ cd ..
$ make stage1-start
$ rm -rf stage3-*
$ echo > ../config/bootstrap-noSFN3.mk 'STAGE3_CFLAGS += 
-gno-statement-frontiers'
$ make -jN BUILD_CONFIG='bootstrap-noSFN3 bootstrap-lto'
$ cd gcc
$ make clean
$ /bin/time make -j1
3587.09user 88.11system 1:01:46elapsed 99%CPU (0avgtext+0avgdata 
948800maxresident)k
0inputs+1937200outputs (0major+46685269minor)pagefaults 0swaps


-- 
Alexandre Oliva, freedom fighterhttp://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [PATCH 2/2] [ARM] Add table of costs for AAarch32 addressing modes.

[Andrew Pinski]

On Fri, Aug 25, 2017 at 10:43 AM, Charles Baylis
 wrote:
> On 9 June 2017 at 15:13, Richard Earnshaw (lists)
>  wrote:
>> On 21/02/17 16:54, charles.bay...@linaro.org wrote:
>>> From: Charles Baylis 
>>>
>>> This patch adds support for modelling the varying costs of
>>> different addressing modes. The generic cost table treats
>>> all addressing modes as having equal cost. The cost table
>>> for Cortex-A57 is derived from 
>>> http://infocenter.arm.com/help/topic/com.arm.doc.uan0015b/Cortex_A57_Software_Optimization_Guide_external.pdf
>>> and treats addressing modes with write-back as having a
>>> cost equal to one additional instruction.
>>>
>>> gcc/ChangeLog:
>>>
>>>   Charles Baylis  
>>>
>>>   * config/arm/aarch-common-protos.h (enum arm_addr_mode_op): New.
>>>   (struct addr_mode_cost_table): New.
>>>   (struct cpu_cost_table): Add pointer to an addr_mode_cost_table.
>>>   * config/arm/aarch-cost-tables.h: (generic_addr_mode_costs): New.
>>>   (generic_extra_costs) Initialise aarch32_addr_mode.
>>>   (cortexa53_extra_costs) Likewise.
>>>   (addr_mode_costs_cortexa57) New.
>>>   (cortexa57_extra_costs) Initialise aarch32_addr_mode.
>>>   (exynosm1_extra_costs) Likewise.
>>>   (xgene1_extra_costs) Likewise.
>>>   (qdf24xx_extra_costs) Likewise.
>>>   * config/arm/arm.c (cortexa9_extra_costs) Initialise 
>>> aarch32_addr_mode.
>>>   (cortexa9_extra_costs) Likewise.
>>>   (cortexa8_extra_costs) Likewise.
>>>   (cortexa5_extra_costs) Likewise.
>>>   (cortexa7_extra_costs) Likewise.
>>>   (cortexa12_extra_costs) Likewise.
>>>   (cortexv7m_extra_costs) Likewise.
>>>   (arm_mem_costs): Use table lookup to calculate cost of addressing
>>>   mode.
>>>
>>> Change-Id: If71bd7c4f4bb876c5ed82dc28791130efb8bf89e
>>> ---
>>>  gcc/config/arm/aarch-common-protos.h | 16 +++
>>>  gcc/config/arm/aarch-cost-tables.h   | 54 
>>> ++
>>>  gcc/config/arm/arm.c | 56 
>>> ++--
>>>  3 files changed, 113 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/gcc/config/arm/aarch-common-protos.h 
>>> b/gcc/config/arm/aarch-common-protos.h
>>> index 7c2bb4c..f6fcc94 100644
>>> --- a/gcc/config/arm/aarch-common-protos.h
>>> +++ b/gcc/config/arm/aarch-common-protos.h
>>> @@ -130,6 +130,21 @@ struct vector_cost_table
>>>const int alu;
>>>  };
>>>
>>> +enum arm_addr_mode_op
>>> +{
>>> +   AMO_DEFAULT,
>>> +   AMO_NO_WB,
>>> +   AMO_WB,
>>> +   AMO_MAX /* for array size */
>>
>> Comment style: Capital letter at start, full stop and two spaces at the end.
>
> Done.
>
>> The enum and structure below should have some comments explaining what
>> they are for.
>
> Done.
>
>>> +const struct addr_mode_cost_table generic_addr_mode_costs =
>>> +{
>>> +  /* int */
>>> +  { 0, 0, 0 },
>>
>> Elements need to be commented, otherwise minor changes to the contents
>> of the arrays is hard to update and maintain.
>
> Done.
>
>
>>> +  /* Addressing mode */
>>
>> Comment style.
>
> Done.
>
>>> -/* TODO: Add table-driven costs for addressing modes.  */
>>> +arm_addr_mode_op op_type;
>>> +switch (GET_CODE(XEXP (x, 0)))
>>> +{
>>> +case REG:
>>> +default:
>>> +  op_type = AMO_DEFAULT;
>>
>> Why would REG and default share the same cost?  Presumably default is
>> too complex to recognize, but then it's unlikely to be cheap.
>
> Default covers literals in various forms of RTL, for which the cost is
> the same as regular, and PIC, which is handled in the original code
> above this section.
>
>>> +  break;
>>> +case PLUS:
>>> +case MINUS:
>>> +  op_type = AMO_NO_WB;
>>
>> GCC doesn't support MINUS in addresses, even though the architecture
>> could in theory handle this.
>
> I've noted that in a comment, but kept the "case MINUS:" in place.
>
>> Also, I think you need a different cost for scaled offset addressing,
>> and possibly even for different scaling factors and types of scaling.
>
> ... see below:
>
>>> +  break;
>>> +case PRE_INC:
>>> +case PRE_DEC:
>>> +case POST_INC:
>>> +case POST_DEC:
>>> +case PRE_MODIFY:
>>> +case POST_MODIFY:
>>> +  op_type = AMO_WB;
>>
>> Pre and post might also need separate entries (plus further entries for
>> scaling).  A post operation might happen in parallel with the data
>> fetch, while a pre operation must happen before the address can be sent
>> to the load/store pipe.
>
> The {DEFAULT, NO_WB, WB} range is also Ramana's requested design. I
> think this is OK because it is sufficient to describe the currently
> publicly documented microarchitectures, and the structure is readily
> extensible if future cores require more detailed modelling.
>
> Expanding the range of AMO_* entries in the array makes the tables
> more unwieldy for no immediate benefit.
>
>>> +  break;
>>> + 

Re: [PATCH 2/2] [ARM] Add table of costs for AAarch32 addressing modes.

[Charles Baylis]

On 9 June 2017 at 15:13, Richard Earnshaw (lists)
 wrote:
> On 21/02/17 16:54, charles.bay...@linaro.org wrote:
>> From: Charles Baylis 
>>
>> This patch adds support for modelling the varying costs of
>> different addressing modes. The generic cost table treats
>> all addressing modes as having equal cost. The cost table
>> for Cortex-A57 is derived from 
>> http://infocenter.arm.com/help/topic/com.arm.doc.uan0015b/Cortex_A57_Software_Optimization_Guide_external.pdf
>> and treats addressing modes with write-back as having a
>> cost equal to one additional instruction.
>>
>> gcc/ChangeLog:
>>
>>   Charles Baylis  
>>
>>   * config/arm/aarch-common-protos.h (enum arm_addr_mode_op): New.
>>   (struct addr_mode_cost_table): New.
>>   (struct cpu_cost_table): Add pointer to an addr_mode_cost_table.
>>   * config/arm/aarch-cost-tables.h: (generic_addr_mode_costs): New.
>>   (generic_extra_costs) Initialise aarch32_addr_mode.
>>   (cortexa53_extra_costs) Likewise.
>>   (addr_mode_costs_cortexa57) New.
>>   (cortexa57_extra_costs) Initialise aarch32_addr_mode.
>>   (exynosm1_extra_costs) Likewise.
>>   (xgene1_extra_costs) Likewise.
>>   (qdf24xx_extra_costs) Likewise.
>>   * config/arm/arm.c (cortexa9_extra_costs) Initialise aarch32_addr_mode.
>>   (cortexa9_extra_costs) Likewise.
>>   (cortexa8_extra_costs) Likewise.
>>   (cortexa5_extra_costs) Likewise.
>>   (cortexa7_extra_costs) Likewise.
>>   (cortexa12_extra_costs) Likewise.
>>   (cortexv7m_extra_costs) Likewise.
>>   (arm_mem_costs): Use table lookup to calculate cost of addressing
>>   mode.
>>
>> Change-Id: If71bd7c4f4bb876c5ed82dc28791130efb8bf89e
>> ---
>>  gcc/config/arm/aarch-common-protos.h | 16 +++
>>  gcc/config/arm/aarch-cost-tables.h   | 54 ++
>>  gcc/config/arm/arm.c | 56 
>> ++--
>>  3 files changed, 113 insertions(+), 13 deletions(-)
>>
>> diff --git a/gcc/config/arm/aarch-common-protos.h 
>> b/gcc/config/arm/aarch-common-protos.h
>> index 7c2bb4c..f6fcc94 100644
>> --- a/gcc/config/arm/aarch-common-protos.h
>> +++ b/gcc/config/arm/aarch-common-protos.h
>> @@ -130,6 +130,21 @@ struct vector_cost_table
>>const int alu;
>>  };
>>
>> +enum arm_addr_mode_op
>> +{
>> +   AMO_DEFAULT,
>> +   AMO_NO_WB,
>> +   AMO_WB,
>> +   AMO_MAX /* for array size */
>
> Comment style: Capital letter at start, full stop and two spaces at the end.

Done.

> The enum and structure below should have some comments explaining what
> they are for.

Done.

>> +const struct addr_mode_cost_table generic_addr_mode_costs =
>> +{
>> +  /* int */
>> +  { 0, 0, 0 },
>
> Elements need to be commented, otherwise minor changes to the contents
> of the arrays is hard to update and maintain.

Done.


>> +  /* Addressing mode */
>
> Comment style.

Done.

>> -/* TODO: Add table-driven costs for addressing modes.  */
>> +arm_addr_mode_op op_type;
>> +switch (GET_CODE(XEXP (x, 0)))
>> +{
>> +case REG:
>> +default:
>> +  op_type = AMO_DEFAULT;
>
> Why would REG and default share the same cost?  Presumably default is
> too complex to recognize, but then it's unlikely to be cheap.

Default covers literals in various forms of RTL, for which the cost is
the same as regular, and PIC, which is handled in the original code
above this section.

>> +  break;
>> +case PLUS:
>> +case MINUS:
>> +  op_type = AMO_NO_WB;
>
> GCC doesn't support MINUS in addresses, even though the architecture
> could in theory handle this.

I've noted that in a comment, but kept the "case MINUS:" in place.

> Also, I think you need a different cost for scaled offset addressing,
> and possibly even for different scaling factors and types of scaling.

... see below:

>> +  break;
>> +case PRE_INC:
>> +case PRE_DEC:
>> +case POST_INC:
>> +case POST_DEC:
>> +case PRE_MODIFY:
>> +case POST_MODIFY:
>> +  op_type = AMO_WB;
>
> Pre and post might also need separate entries (plus further entries for
> scaling).  A post operation might happen in parallel with the data
> fetch, while a pre operation must happen before the address can be sent
> to the load/store pipe.

The {DEFAULT, NO_WB, WB} range is also Ramana's requested design. I
think this is OK because it is sufficient to describe the currently
publicly documented microarchitectures, and the structure is readily
extensible if future cores require more detailed modelling.

Expanding the range of AMO_* entries in the array makes the tables
more unwieldy for no immediate benefit.

>> +  break;
>> +}
>
> blank line between the switch block and the subsequent statements.
> Also, the following needs to laid out in GNU style.

Done.

This version of the patch also moves the Cortex-A57 tuning (which was
present in the earlier version) into a 

Re: [PATCH 1/2] [ARM] Refactor costs calculation for MEM.

[Charles Baylis]

On 9 June 2017 at 14:59, Richard Earnshaw (lists)
 wrote:
> On 21/02/17 16:54, charles.bay...@linaro.org wrote:
>> From: Charles Baylis 
>>
>> This patch moves the calculation of costs for MEM into a
>> separate function, and reforms the calculation into two
>> parts. Firstly any additional cost of the addressing mode
>> is calculated, and then the cost of the memory access itself
>> is added.
>>
>> In this patch, the calculation of the cost of the addressing
>> mode is left as a placeholder, to be added in a subsequent
>> patch.
>>
>> gcc/ChangeLog:
>>
>>   Charles Baylis  
>>
>> * config/arm/arm.c (arm_mem_costs): New function.
>> (arm_rtx_costs_internal): Use arm_mem_costs.
>
> I like the idea of this patch, but it needs further work...
>
> Comments inline.
>
> R.
>
>>
>> Change-Id: I99e93406ea39ee31f71c7bf428ad3e127b7a618e
>> ---
>>  gcc/config/arm/arm.c | 66 
>> +---
>>  1 file changed, 42 insertions(+), 24 deletions(-)
>>
>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>> index 6cae178..7f002f1 100644
>> --- a/gcc/config/arm/arm.c
>> +++ b/gcc/config/arm/arm.c
>> @@ -9072,6 +9072,47 @@ arm_unspec_cost (rtx x, enum rtx_code /* outer_code 
>> */, bool speed_p, int *cost)
>> } \
>>   while (0);
>>
>> +/* Helper function for arm_rtx_costs_internal. Calculates the cost of a MEM,
>> +   considering the costs of the addressing mode and memory access
>> +   separately.  */
>> +static bool
>> +arm_mem_costs (rtx x, const struct cpu_cost_table *extra_cost,
>> +int *cost, bool speed_p)
>> +{
>> +  machine_mode mode = GET_MODE (x);
>> +  if (flag_pic
>> +  && GET_CODE (XEXP (x, 0)) == PLUS
>> +  && will_be_in_index_register (XEXP (XEXP (x, 0), 1)))
>> +/* This will be split into two instructions.  Add the cost of the
>> +   additional instruction here.  The cost of the memory access is 
>> computed
>> +   below.  See arm.md:calculate_pic_address.  */
>> +*cost = COSTS_N_INSNS (1);
>> +  else
>> +*cost = 0;
>> +
>> +  /* Calculate cost of the addressing mode.  */
>> +  if (speed_p)
>> +  {
>
> This patch needs to be reformatted in the GNU style (indentation of
> braces, braces and else clauses on separate lines etc).

Done.

>> +/* TODO: Add table-driven costs for addressing modes.  */
>
> You need to sort out the comment.  What's missing here?

What's missing is patch 2... I've updated the comment for clarity.

>> +  }
>> +
>> +  /* cost of memory access */
>> +  if (speed_p)
>> +  {
>> +/* data transfer is transfer size divided by bus width.  */
>> +int bus_width = arm_arch7 ? 8 : 4;
>
> Basing bus width on the architecture is a bit too simplistic.  Instead
> this should be a parameter that comes from the CPU cost tables, based on
> the current tune target.

This was actually Ramana's suggestion, so I've left it as-is in this
patch. If necessary, I think it's better to move this to a table in a
separate patch, as I'll need to guess the correct bus width for a
number of CPUs and will probably get some wrong.

>> +*cost += COSTS_N_INSNS((GET_MODE_SIZE (mode) + bus_width - 1) / 
>> bus_width);
>
> Use CEIL (from system.h)

Done.

Updated patch attached.
From 18629835ba12fdfa693e2f9492a5fc23d95ef165 Mon Sep 17 00:00:00 2001
From: Charles Baylis 
Date: Wed, 8 Feb 2017 16:52:10 +
Subject: [PATCH 1/3] [ARM] Refactor costs calculation for MEM.

This patch moves the calculation of costs for MEM into a
separate function, and reforms the calculation into two
parts. Firstly any additional cost of the addressing mode
is calculated, and then the cost of the memory access itself
is added.

In this patch, the calculation of the cost of the addressing
mode is left as a placeholder, to be added in a subsequent
patch.

gcc/ChangeLog:

  Charles Baylis  

* config/arm/arm.c (arm_mem_costs): New function.
(arm_rtx_costs_internal): Use arm_mem_costs.

Change-Id: I99e93406ea39ee31f71c7bf428ad3e127b7a618e
---
 gcc/config/arm/arm.c | 67 
 1 file changed, 42 insertions(+), 25 deletions(-)

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index fa3e2fa..13cd421 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -9198,8 +9198,48 @@ arm_unspec_cost (rtx x, enum rtx_code /* outer_code */, bool speed_p, int *cost)
 	  }\
 	while (0);
 
+/* Helper function for arm_rtx_costs_internal.  Calculates the cost of a MEM,
+   considering the costs of the addressing mode and memory access
+   separately.  */
+static bool
+arm_mem_costs (rtx x, const struct cpu_cost_table *extra_cost,
+	   int *cost, bool speed_p)
+{
+  machine_mode mode = GET_MODE (x);
+  if (flag_pic
+  && GET_CODE (XEXP (x, 0)) == PLUS
+  

Re: [PATCH] handle pathological anti-ranges in gimple_fold_builtin_memory_op (PR 81908)

[Richard Sandiford]

Martin Sebor  writes:
>> Just a stylistic thing, but since the only use of "wone" is in
>> the eq_p, it'd be simpler just to use "1".  Also, the maximum
>> value is better calculated as "wi::max_value (prec, SIGNED)".  So:
>>
>>   /* Compute the value of SSIZE_MAX, the largest positive value that
>>  can be stored in ssize_t, the signed counterpart of size_t .  */
>>   wide_int ssize_max = wi::max_value (prec, SIGNED);
>>
>>   return wi::eq_p (min, 1) && wi::geu_p (max, ssize_max);
>
> Thanks, I didn't know about the implicit conversion or the max_value
> API.  I'll remember to use them the next time.
>
> FWIW, going slightly off topic, and while I'm sure it's a matter
> of getting used to the design, I can't say I find the wide_int
> classes exactly intuitive.  It seems that the most natural way
> to write the return statement in C++ is:
>
>return min == 1 && max >= ssize_max;
>
> The first subexpression is accepted but the second one doesn't even
> compile.  If it did, it would treat the operands as signed which
> isn't what I need here.  One still has to resort to the clunky
> predicate for it:
>
>return min == 1 && wi::geu_p (max, ssize_max);

There are two main reasons for the wi:: stuff:

1. When we were adding wide-int initially, one of the requirements
   was that we could operate directly on rtx and tree representations
   of integers, without first converting them to wide_int etc.
   Since trees and rtxes are pointers, it wouldn't make sense
   to rely on operator overloading there.

2. wide_int itself has no inherent sign.  (So I disagree with
   "If it did, it would treat the operands as signed which isn't what
   I need here."  The reason for not having the overload is exactly
   that there is no obvious choice between signed and unsigned here.)

And yes, Richard has been giving me grief about the lack of a
wide_int-with-sign :-)

> It also doesn't help that the names of the predicates don't follow
> the same convention as those that operate on trees (e.g., eq_p vs
> tree_int_cst_equal).
>
> Unless there's some inherent limitation that makes it impossible
> to use the class the way I would like it might be worth investing
> some time into making it more user-friendly.

I think we already provide the overloads we can (i.e. those where
the sign is known or isn't relevant).  Let me know if we've missed
some though.

Thanks,
Richard

Re: [PATCH 1/2] Don't warn function alignment if warn_if_not_aligned_p is true

[H.J. Lu]

On Fri, Aug 25, 2017 at 10:15 AM, Martin Sebor  wrote:
> On 08/21/2017 06:21 AM, H.J. Lu wrote:
>>
>> When warn_if_not_aligned_p is true, a warning will be issued on function
>> declaration later.  There is no need to warn function alignment when
>> warn_if_not_aligned_p is true.
>>
>> OK for trunk?
>>
>> H.J.
>> --
>> * c-attribs.c (common_handle_aligned_attribute): Don't warn
>> function alignment if warn_if_not_aligned_p is true.
>> ---
>>  gcc/c-family/c-attribs.c | 5 -
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
>> index 5f79468407f..78969532543 100644
>> --- a/gcc/c-family/c-attribs.c
>> +++ b/gcc/c-family/c-attribs.c
>> @@ -1754,9 +1754,12 @@ common_handle_aligned_attribute (tree *node, tree
>> args, int flags,
>>This formally comes from the c++11 specification but we are
>>doing it for the GNU attribute syntax as well.  */
>>  *no_add_attrs = true;
>> -  else if (TREE_CODE (decl) == FUNCTION_DECL
>> +  else if (!warn_if_not_aligned_p
>> +  && TREE_CODE (decl) == FUNCTION_DECL
>>&& DECL_ALIGN (decl) > (1U << i) * BITS_PER_UNIT)
>>  {
>> +  /* Don't warn function alignment here if warn_if_not_aligned_p is
>> +true.  It will be warned later.  */
>>if (DECL_USER_ALIGN (decl))
>> error ("alignment for %q+D was previously specified as %d "
>>"and may not be decreased", decl,
>
>
> Your comment refers to warning but the code here uses error().
> That raises two questions for me: a) will the later diagnostic
> really be a warning or an error, and if a warning, under what
> option will it be issued? and b) why is an error appropriate
> here when a warning is appropriate elsewhere (most other
> attribute conflicts are at present diagnosed with -Wattributes).

It can be changed to warning.

> My main motivation for these questions is to understand the
> rationale for warning for vs rejecting conflicts so that
> a consistent general solution can be implemented for all
> attributes (i.e., along the lines of my patch here:
> https://gcc.gnu.org/ml/gcc-patches/2017-08/msg01457.html)
>
> Martin



-- 
H.J.

Re: [PATCH 1/2] Don't warn function alignment if warn_if_not_aligned_p is true

[Martin Sebor]

On 08/21/2017 06:21 AM, H.J. Lu wrote:

When warn_if_not_aligned_p is true, a warning will be issued on function
declaration later.  There is no need to warn function alignment when
warn_if_not_aligned_p is true.

OK for trunk?

H.J.
--
* c-attribs.c (common_handle_aligned_attribute): Don't warn
function alignment if warn_if_not_aligned_p is true.
---
 gcc/c-family/c-attribs.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 5f79468407f..78969532543 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -1754,9 +1754,12 @@ common_handle_aligned_attribute (tree *node, tree args, 
int flags,
   This formally comes from the c++11 specification but we are
   doing it for the GNU attribute syntax as well.  */
 *no_add_attrs = true;
-  else if (TREE_CODE (decl) == FUNCTION_DECL
+  else if (!warn_if_not_aligned_p
+  && TREE_CODE (decl) == FUNCTION_DECL
   && DECL_ALIGN (decl) > (1U << i) * BITS_PER_UNIT)
 {
+  /* Don't warn function alignment here if warn_if_not_aligned_p is
+true.  It will be warned later.  */
   if (DECL_USER_ALIGN (decl))
error ("alignment for %q+D was previously specified as %d "
   "and may not be decreased", decl,


Your comment refers to warning but the code here uses error().
That raises two questions for me: a) will the later diagnostic
really be a warning or an error, and if a warning, under what
option will it be issued? and b) why is an error appropriate
here when a warning is appropriate elsewhere (most other
attribute conflicts are at present diagnosed with -Wattributes).

My main motivation for these questions is to understand the
rationale for warning for vs rejecting conflicts so that
a consistent general solution can be implemented for all
attributes (i.e., along the lines of my patch here:
https://gcc.gnu.org/ml/gcc-patches/2017-08/msg01457.html)

Martin

Re: [77/77] Add a complex_mode class

[Jeff Law]

On 07/13/2017 03:05 AM, Richard Sandiford wrote:
> This patch adds another machine_mode wrapper for modes that are
> known to be COMPLEX_MODE_P.  There aren't yet many places that make
> use of it, but that might change in future.
> 
> 2017-07-13  Richard Sandiford  
>   Alan Hayward  
>   David Sherwood  
> 
> gcc/
>   * coretypes.h (complex_mode): New type.
>   * gdbhooks.py (build_pretty_printer): Handle it.
>   * machmode.h (complex_mode): New class.
>   (complex_mode::includes_p): New function.
>   (is_complex_int_mode): Likewise.
>   (is_complex_float_mode): Likewise.
>   * genmodes.c (get_mode_class): Handle complex mode classes.
>   * function.c (expand_function_end): Use is_complex_int_mode.
> 
> gcc/go/
>   * go-lang.c (go_langhook_type_for_mode): Use is_complex_float_mode.
OK.

I think that's the whole set.  I want to go back and review the API
issue raised early in the kit, but otherwise I don't see any concerns.

Obviously the trunk has continued to evolve since the kit was posted.
It's safe to assume there'll be minor updates as a result -- I trust
your judgment on addressing any such fallout without going through
another review cycle.

Jeff

Re: Add a partial_integral_type_p helper function

[Martin Sebor]

On 08/18/2017 02:13 AM, Richard Sandiford wrote:

This patch adds a partial_integral_type_p function, to go along
with the full_integral_type_p added by the previous patch.

Of the changes that didn't previously have an INTEGRAL_TYPE_P check:

- the convert_to_integer_1 hunks are dominated by a case version
  of INTEGRAL_TYPE_P.

- the merge_ranges hunk is dominated by an ENUMERAL_TYPE case.

- vectorizable_reduction has the comment:

  /* Do not try to vectorize bit-precision reductions.  */

  and so I think was only concerned with integers.

- vectorizable_assignment has the comment:

  /* We do not handle bit-precision changes.  */

  and the later:

  /* But a conversion that does not change the bit-pattern is ok.  */
  && !((TYPE_PRECISION (TREE_TYPE (scalar_dest))
> TYPE_PRECISION (TREE_TYPE (op)))
   && TYPE_UNSIGNED (TREE_TYPE (op)))

  would only make sense if OP is also an integral type.

- vectorizable_shift is inherently restricted to integers.

Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?

Richard


2017-08-17  Richard Sandiford  
Alan Hayward  
David Sherwood  

gcc/
* tree.h (partial_integral_type_p): New function.
* convert.c (convert_to_integer_1): Use it.
* expr.c (store_fieldexpand_expr_real_2, expand_expr_real_1): Likewise.
* fold-const.c (merge_ranges): Likewise.
* tree-ssa-math-opts.c (convert_mult_to_fma): Likewise.
* tree-tailcall.c (process_assignment): Likewise.
* tree-vect-loop.c (vectorizable_reduction): Likewise.
* tree-vect-stmts.c (vectorizable_conversion): Likewise.
(vectorizable_assignment, vectorizable_shift): Likewise.

Index: gcc/tree.h
===
--- gcc/tree.h  2017-08-18 08:35:58.031690315 +0100
+++ gcc/tree.h  2017-08-18 08:36:07.208306339 +0100
@@ -5414,4 +5414,13 @@ full_integral_type_p (const_tree t)
   return INTEGRAL_TYPE_P (t) && scalar_type_is_full_p (t);
 }

+/* Return true if T is an integral type that has fewer bits than
+   its underlying mode.  */


May I suggest to rephrase that as "has fewer bits of precision
than..." to make it clear what those bits refer to?  Alternatively.
"type whose precision is less than that of its underlying mode."


+
+inline bool
+partial_integral_type_p (const_tree t)
+{
+  return INTEGRAL_TYPE_P (t) && !scalar_type_is_full_p (t);
+}
+


Martin

Re: [76/77] Add a scalar_mode_pod class

[Jeff Law]

On 07/13/2017 03:05 AM, Richard Sandiford wrote:
> This patch adds a scalar_mode_pod class and uses it to
> replace the machine_mode in fixed_value.
> 
> 2017-07-13  Richard Sandiford  
>   Alan Hayward  
>   David Sherwood  
> 
> gcc/
>   * coretypes.h (scalar_mode_pod): New typedef.
>   * gdbhooks.py (build_pretty_printer): Handle it.
>   * machmode.h (gt_ggc_mx, gt_pch_nx): New functions.
>   * fixed-value.h (fixed_value::mode): Change type to scalar_mode_pod.
>   * fold-const.c (fold_convert_const_int_from_fixed): Use scalar_mode.
>   * tree-streamer-in.c (unpack_ts_fixed_cst_value_fields): Use
>   as_a .
OK.
jeff

[PATCH] Fix fortran vector tests on powerpc and aarch64, PR tree-optimization/80925

[Steve Ellcey]

My earlier patch to update tests and resolve PR tree-optimization/80925
did not include FORTRAN, just C and C++.  This patch makes the same
changes as the earlier patches but for FORTRAN.  Tested on aarch64.

OK to checkin?

Steve Ellcey
sell...@cavium.com

Orginal patches/discussion is at:

https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01862.html
https://gcc.gnu.org/ml/gcc-patches/2017-08/msg0.html


2017-08-25  Steve Ellcey  

PR tree-optimization/80925
* gfortran.dg/vect/vect-2.f90: Add
--param vect-max-peeling-for-alignment=0 option.
Remove unaligned access and peeling checks.
* gfortran.dg/vect/vect-3.f90: Ditto.
* gfortran.dg/vect/vect-4.f90: Ditto.
* gfortran.dg/vect/vect-5.f90: Ditto.

diff --git a/gcc/testsuite/gfortran.dg/vect/vect-2.f90 b/gcc/testsuite/gfortran.dg/vect/vect-2.f90
index 24c7deb..956a05c 100644
--- a/gcc/testsuite/gfortran.dg/vect/vect-2.f90
+++ b/gcc/testsuite/gfortran.dg/vect/vect-2.f90
@@ -1,5 +1,6 @@
 ! { dg-do compile }
 ! { dg-require-effective-target vect_float }
+! { dg-additional-options "--param vect-max-peeling-for-alignment=0" }
 
 SUBROUTINE FOO(A, B, C)
 DIMENSION A(100), B(100), C(100)
@@ -15,7 +16,4 @@ END
 ! support unaligned loads).
 
 ! { dg-final { scan-tree-dump-times "vectorized 3 loops" 1 "vect" } }
-! { dg-final { scan-tree-dump-times "Alignment of access forced using peeling" 3 "vect" { xfail { { vect_no_align && { ! vect_hw_misalign } } || { ! vector_alignment_reachable } } } } }
-! { dg-final { scan-tree-dump-times "Alignment of access forced using peeling" 2 "vect" { target { { vect_no_align && { ! vect_hw_misalign } } && { ! vector_alignment_reachable } } } } }
-! { dg-final { scan-tree-dump-times "Vectorizing an unaligned access" 2 "vect" { xfail { vect_no_align && { ! vect_hw_misalign } } } } }
 ! { dg-final { scan-tree-dump-times "Alignment of access forced using versioning." 3 "vect" {target { { vect_no_align && { ! vect_hw_misalign } } || { { ! vector_alignment_reachable  } && { ! vect_hw_misalign } } } } } }
diff --git a/gcc/testsuite/gfortran.dg/vect/vect-3.f90 b/gcc/testsuite/gfortran.dg/vect/vect-3.f90
index abbdc90..b8ccbdd 100644
--- a/gcc/testsuite/gfortran.dg/vect/vect-3.f90
+++ b/gcc/testsuite/gfortran.dg/vect/vect-3.f90
@@ -1,5 +1,6 @@
 ! { dg-do compile }
 ! { dg-require-effective-target vect_float }
+! { dg-additional-options "--param vect-max-peeling-for-alignment=0" }
 
 SUBROUTINE SAXPY(X, Y, A, N)
 DIMENSION X(N), Y(N)
@@ -8,7 +9,4 @@ END
 
 ! { dg-final { scan-tree-dump-times "Alignment of access forced using versioning" 3 "vect" { target { vect_no_align && { ! vect_hw_misalign } } } } }
 ! { dg-final { scan-tree-dump-times "Alignment of access forced using versioning" 1 "vect" { target { {! vect_no_align} && { {! vector_alignment_reachable} && {! vect_hw_misalign} } } } } }
-! { dg-final { scan-tree-dump-times "Vectorizing an unaligned access" 2 "vect" { target { {! vect_no_align} && { {! vector_alignment_reachable} && {! vect_hw_misalign} } } } } }
-! { dg-final { scan-tree-dump-times "Alignment of access forced using peeling" 1 "vect" { xfail { { vect_no_align && { ! vect_hw_misalign } } || {! vector_alignment_reachable}} } } }
-! { dg-final { scan-tree-dump-times "Vectorizing an unaligned access" 1 "vect" { xfail { { vect_no_align && { ! vect_hw_misalign } } || { ! vector_alignment_reachable} } } } }
 
diff --git a/gcc/testsuite/gfortran.dg/vect/vect-4.f90 b/gcc/testsuite/gfortran.dg/vect/vect-4.f90
index 7602100..b567cbd 100644
--- a/gcc/testsuite/gfortran.dg/vect/vect-4.f90
+++ b/gcc/testsuite/gfortran.dg/vect/vect-4.f90
@@ -1,5 +1,6 @@
 ! { dg-do compile }
 ! { dg-require-effective-target vect_float }
+! { dg-additional-options "--param vect-max-peeling-for-alignment=0" }
 
 ! Peeling to align the store to Y will also align the load from Y.
 ! The load from X may still be misaligned.
@@ -10,7 +11,4 @@ Y = Y + A * X
 END
 
 ! { dg-final { scan-tree-dump-times "vectorized 1 loops" 1 "vect" } } 
-! { dg-final { scan-tree-dump-times "Alignment of access forced using peeling" 1 "vect" { xfail { { vect_no_align && { ! vect_hw_misalign } } || {! vector_alignment_reachable} } } } }
-! { dg-final { scan-tree-dump-times "Vectorizing an unaligned access" 1 "vect" { xfail { { vect_no_align && { ! vect_hw_misalign } } || {! vector_alignment_reachable} } } } }
-! { dg-final { scan-tree-dump-times "Vectorizing an unaligned access" 2 "vect" { target { {! vector_alignment_reachable} && {! vect_hw_misalign} } } } }
 ! { dg-final { scan-tree-dump-times "accesses have the same alignment." 1 "vect" } }
diff --git a/gcc/testsuite/gfortran.dg/vect/vect-5.f90 b/gcc/testsuite/gfortran.dg/vect/vect-5.f90
index 4c6324e..54887f9 100644
--- a/gcc/testsuite/gfortran.dg/vect/vect-5.f90
+++ b/gcc/testsuite/gfortran.dg/vect/vect-5.f90
@@ -1,4 +1,5 @@
 ! { dg-require-effective-target vect_int }
+! { dg-additional-options "--param vect-max-peeling-for-alignment=0" }
 

Re: [74/77] Various small scalar_mode changes

[Jeff Law]

On 07/13/2017 03:04 AM, Richard Sandiford wrote:
> This patch uses scalar_mode in a few miscellaneous places:
> 
> - Previous patches mean mode_to_vector can take a scalar_mode without
>   further changes.
> 
> - Implicit promotion is limited to scalar types (affects promote_mode
>   and sdbout_parms)
> 
> 2017-07-13  Richard Sandiford  
>   Alan Hayward  
>   David Sherwood  
> 
> gcc/
>   * machmode.h (mode_for_vector): Take a scalar_mode instead
>   of a machine_mode.
>   * stor-layout.c (mode_for_vector): Likewise.
>   * explow.c (promote_mode): Use as_a .
>   * sdbout.c (sdbout_parms): Use is_a .
OK.
jeff

Re: [75/77] Use scalar_mode in the AArch64 port

[Jeff Law]

On 07/13/2017 03:04 AM, Richard Sandiford wrote:
> Similar to the previous scalar_int_mode patch.
> 
> 2017-07-13  Richard Sandiford  
>   Alan Hayward  
>   David Sherwood  
> 
> gcc/
>   * config/aarch64/aarch64-protos.h (aarch64_gen_adjusted_ldpstp):
>   Take a scalar_mode rather than a machine_mode.
>   (aarch64_operands_adjust_ok_for_ldpstp): Likewise.
>   * config/aarch64/aarch64.c (aarch64_simd_container_mode): Likewise.
>   (aarch64_operands_adjust_ok_for_ldpstp): Likewise.
>   (aarch64_gen_adjusted_ldpstp): Likewise.
>   (aarch64_expand_vector_init): Use scalar_mode instead of machine_mode.
I'll leave this to the aarch64 folks.

jeff

Re: [73/77] Pass scalar_mode to scalar_mode_supported_p

[Jeff Law]

On 07/13/2017 03:04 AM, Richard Sandiford wrote:
> This patch makes the preferred_simd_mode target hook take a scalar_mode
> rather than a machine_mode.
> 
> 2017-07-13  Richard Sandiford  
>   Alan Hayward  
>   David Sherwood  
> 
> gcc/
>   * target.def (preferred_simd_mode): Take a scalar_mode
>   instead of a machine_mode.
>   * targhooks.h (default_preferred_simd_mode): Likewise.
>   * targhooks.c (default_preferred_simd_mode): Likewise.
>   * config/aarch64/aarch64.c (aarch64_preferred_simd_mode): Likewise.
>   * config/arc/arc.c (arc_preferred_simd_mode): Likewise.
>   * config/arm/arm.c (arm_preferred_simd_mode): Likewise.
>   * config/c6x/c6x.c (c6x_preferred_simd_mode): Likewise.
>   * config/epiphany/epiphany.c (epiphany_preferred_simd_mode): Likewise.
>   * config/i386/i386.c (ix86_preferred_simd_mode): Likewise.
>   * config/mips/mips.c (mips_preferred_simd_mode): Likewise.
>   * config/powerpcspe/powerpcspe.c (rs6000_preferred_simd_mode):
>   Likewise.
>   * config/rs6000/rs6000.c (rs6000_preferred_simd_mode): Likewise.
>   * config/s390/s390.c (s390_preferred_simd_mode): Likewise.
>   * config/sparc/sparc.c (sparc_preferred_simd_mode): Likewise.
>   * doc/tm.texi: Regenerate.
>   * optabs-query.c (can_vec_mask_load_store_p): Return false for
>   non-scalar modes.
OK.
jeff

Re: [72/77] Pass scalar_mode to scalar_mode_supported_p

[Jeff Law]

On 07/13/2017 03:04 AM, Richard Sandiford wrote:
> This patch makes the scalar_mode_supported_p target hook take a
> scalar_mode rather than a machine_mode.
> 
> 2017-07-13  Richard Sandiford  
>   Alan Hayward  
>   David Sherwood  
> 
> gcc/
>   * target.def (scalar_mode_supported_p): Take a scalar_mode
>   instead of a machine_mode.
>   * targhooks.h (default_scalar_mode_supported_p): Likewise.
>   * targhooks.c (default_scalar_mode_supported_p): Likewise.
>   * config/aarch64/aarch64.c (aarch64_scalar_mode_supported_p): Likewise.
>   * config/alpha/alpha.c (alpha_scalar_mode_supported_p): Likewise.
>   * config/arm/arm.c (arm_scalar_mode_supported_p): Likewise.
>   * config/avr/avr.c (avr_scalar_mode_supported_p): Likewise.
>   * config/c6x/c6x.c (c6x_scalar_mode_supported_p): Likewise.
>   * config/i386/i386.c (ix86_scalar_mode_supported_p): Likewise.
>   * config/ia64/ia64.c (ia64_scalar_mode_supported_p): Likewise.
>   * config/mips/mips.c (mips_scalar_mode_supported_p): Likewise.
>   * config/msp430/msp430.c (msp430_scalar_mode_supported_p): Likewise.
>   * config/pa/pa.c (pa_scalar_mode_supported_p): Likewise.
>   * config/pdp11/pdp11.c (pdp11_scalar_mode_supported_p): Likewise.
>   * config/powerpcspe/powerpcspe.c (rs6000_scalar_mode_supported_p):
>   Likewise.
>   * config/rs6000/rs6000.c (rs6000_scalar_mode_supported_p): Likewise.
>   * config/s390/s390.c (s390_scalar_mode_supported_p): Likewise.
>   * config/spu/spu.c (spu_scalar_mode_supported_p): Likewise.
>   * config/tilegx/tilegx.c (tilegx_scalar_mode_supported_p): Likewise.
>   * config/tilepro/tilepro.c (tilepro_scalar_mode_supported_p):
>   Likewise.
>   * doc/tm.texi: Regenerate.
> 
> gcc/c-family/
>   * c-attribs.c (vector_mode_valid_p) Fold GET_MODE_INNER call
>   into scalar_mode_supported_p call.
>   (handle_mode_attribute): Update call to scalar_mode_supported_p.
> 
OK.
jeff

Re: [71/77] Use opt_scalar_mode for mode iterators

[Jeff Law]

On 07/13/2017 03:03 AM, Richard Sandiford wrote:
> This patch uses opt_scalar_mode when iterating over scalar modes.
> 
> 2017-07-13  Richard Sandiford  
>   Alan Hayward  
>   David Sherwood  
> 
> gcc/
>   * coretypes.h (opt_scalar_mode): New typedef.
>   * gdbhooks.py (build_pretty_printers): Handle it.
>   * machmode.h (mode_iterator::get_2xwider): Add overload for
>   opt_mode.
>   * emit-rtl.c (init_emit_once): Use opt_scalar_mode when iterating
>   over scalar modes.
>   * expr.c (convert_mode_scalar): Likewise.
>   * omp-low.c (omp_clause_aligned_alignment): Likewise.
>   * optabs.c (expand_float): Likewise.
>   (expand_fix): Likewise.
>   * tree-vect-stmts.c (vectorizable_conversion): Likewise.
> 
> gcc/c-family/
>   * c-common.c (c_common_fixed_point_type_for_size): Use opt_scalar_mode
>   for the mode iterator.
OK.
jeff

Re: [PATCH] Fix file find utils and add unit tests (PR driver/81829).

[Martin Sebor]

On 08/18/2017 04:17 AM, Martin Liška wrote:

On 08/15/2017 02:45 PM, Martin Liška wrote:

Hi.

As shown in the PR, remove_prefix function is written wrongly. It does not 
distinguish
in between a node in linked list and pprefix->plist. So I decide to rewrite it.
Apart from that, I identified discrepancy in between do_add_prefix and 
prefix_from_string
where the later one appends always DIR_SEPARATOR (if missing). So I also the 
former function.
And I'm adding unit tests for all functions in file-find.c.


I know only very little about this API but from a quick glance at
the change it looks to me like it introduces an implicit assumption
that prefix points to a non-empty string.  If that is in fact one
of the function's preconditions I would suggest to a) assert it
before relying on it, and b) document it.  Otherwise, if the prefix
is allowed to be empty then the code below is undefined in that
case.

Martin

@@ -126,11 +127,22 @@ do_add_prefix (struct path_prefix *pprefix, const 
char *prefix, bool first)

   /* Keep track of the longest prefix.  */

   len = strlen (prefix);
+  bool append_separator = !IS_DIR_SEPARATOR (prefix[len - 1]);
+  if (append_separator)
+len++;
+
   if (len > pprefix->max_le

Re: [PATCH] handle pathological anti-ranges in gimple_fold_builtin_memory_op (PR 81908)

[Martin Sebor]

Just a stylistic thing, but since the only use of "wone" is in
the eq_p, it'd be simpler just to use "1".  Also, the maximum
value is better calculated as "wi::max_value (prec, SIGNED)".  So:

  /* Compute the value of SSIZE_MAX, the largest positive value that
 can be stored in ssize_t, the signed counterpart of size_t .  */
  wide_int ssize_max = wi::max_value (prec, SIGNED);

  return wi::eq_p (min, 1) && wi::geu_p (max, ssize_max);


Thanks, I didn't know about the implicit conversion or the max_value
API.  I'll remember to use them the next time.

FWIW, going slightly off topic, and while I'm sure it's a matter
of getting used to the design, I can't say I find the wide_int
classes exactly intuitive.  It seems that the most natural way
to write the return statement in C++ is:

  return min == 1 && max >= ssize_max;

The first subexpression is accepted but the second one doesn't even
compile.  If it did, it would treat the operands as signed which
isn't what I need here.  One still has to resort to the clunky
predicate for it:

  return min == 1 && wi::geu_p (max, ssize_max);

It also doesn't help that the names of the predicates don't follow
the same convention as those that operate on trees (e.g., eq_p vs
tree_int_cst_equal).

Unless there's some inherent limitation that makes it impossible
to use the class the way I would like it might be worth investing
some time into making it more user-friendly.

Martin

Re: [pr 81982] wide-int.h change broke native arm-* gcc

[Szabolcs Nagy]

On 25/08/17 14:27, Szabolcs Nagy wrote:
> i don't see corresponding email thread in gcc-patches, but
> since this commit:
> https://gcc.gnu.org/viewcvs/gcc?view=revision=251260
> 
> i see arm-* native compiler miscompile libstdc++,
> note that the cross compiler compiles it correctly.
> 
> i guess it can be some latent bug that got uncovered,
> but it is reliably reproducible, i reported
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81982
> 
> those who run native arm gcc tests may see c++ execution tests
> timeout while spinning in __cxa_guard_acquire around a futex
> syscall.
> 
> i'm still debugging it, but ideas and help is welcome.
> 

it was a dup and already fixed.
sorry for the noise

Re: [Patch][aarch64] Use IFUNCs to enable LSE instructions in libatomic on aarch64

[Szabolcs Nagy]

On 07/08/17 21:44, Steve Ellcey wrote:
> This patch uses the libatomic IFUNC infrastructure so that aarch64
> machines that support the LSE instructions can use them.  Note that
> aarch64 still isn't enabling IFUNC support by default though I have
> submitted a patch to do that.  You can enable IFUNC support by
> configuring with --enable-gnu-indirect-function.
> 
> Glibc has an environment variable, LD_HWCAP_MASK, that can be used to
> mask out some or all of the bits returned by getauxval(AT_HWCAP) and
> ignore certain hardware capabilities.  I enabled this functionality
> for libatomic by looking at the LD_HWCAP_MASK variable in the IFUNC
> resolver function.  That way, if I had a system that supported LSE but
> did not want to use it for some reason, I could set LD_HWCAP_MASK to
> zero and then the IFUNC selector function would not enable the LSE
> routines.  I could remove this functionality if we thought it was not
> appropriate but I think it is useful, both for testing and for end
> users.
> 

the use of ifunc in gcc target libraries was a mistake
in my opinion, there are several known bugs in the ifunc
design and uclibc/musl/bionic don't plan to support it.
but at this point i dont have a better proposal for doing
runtime selection of optimal atomics code.

however in this patch i don't see why would the ctor run
before ifunc resolvers. how does this work on x86_64?
(there the different 16byte atomics are not even compatible,
so if ifunc resolvers in different dsos return different
result because one ran before the ctor, another after then
the runtime behaviour is broken. this can happen when one
dso is bindnow so ifunc relocation is processed before
ctors and another runs resolvers lazily or dlopened later..
but i didnt test it just looks broken)

note that aarch64 ifunc resolvers get hwcap as an argument
so all this brokenness can be avoided (and if we want to
disable hwcap flags then probably glibc should take care
of that before passing hwcap to the ifunc resolver).

> Tested on aarch64, OK for checkin?
> 
> Steve Ellcey
> sell...@cavium.com
> 
> 
> 
> 
> 2017-08-07  Steve Ellcey  
> 
>   * Makefile.am (ARCH_AARCH64_LINUX_LSE): Add IFUNC_OPTIONS and
>   libatomic_la_LIBADD.
>   * config/linux/aarch64/host-config.h: New file.
>   * config/linux/aarch64/init.c: New file.
>   * configure.ac (AC_CHECK_HEADERS): Check for sys/auxv.h.
>   (AC_CHECK_FUNCS): Check for getauxval.
>   (ARCH_AARCH64_LINUX_LSE): New conditional for IFUNC builds.
>   * configure.tgt (aarch64): Set AARCH and try_ifunc.
>   (aarch64*-*-linux*) Update config_path.
>   * Makefile.in: Regenerate.
>   * auto-config.h.in: Regenerate.
>   * configure: Regenerate.
> 

Re: Statement Frontier Notes, Location Views, and Inlined Entry Point Markers

[Alexandre Oliva]

On Aug 23, 2017, Richard Biener  wrote:

> On Wed, Aug 23, 2017 at 12:29 AM, Alexandre Oliva  wrote:
>> All debug options that support negation seem to have adopted this idiom;
>> without it, the negated options end up misparsed as -g with an argument,
>> and then set_debug_level complains that "no-..." is not a number.
>> 
>> The logic of matching the longest option name prefix doesn't seem to
>> work very well when options have a prefix that is also a valid option
>> with a Joined(OrMissing) argument.

> Ah, like we don't have -no-g but only -g0.  I guess that there's -fno-
> is somewhere hardcoded and -g isn't handled that way.  Would need
> to amend the options machinery somehow (new flag, Negatable?).

This patch that adds -g to the set of negatable prefixes along with -f,
-m and -W.  Besides the mapping from -gno- to negated -g in option_map
and adding g to the [fmW] matches for negatable options, I had to
introduce gno- as an remapping prefix, for the option searching
machinery to backtrack to and recognize as a remapping prefix, instead
of backtracking to -g and stopping at it as if no-* was its Joined
argument.  Adding such remapping prefixes to preempt further
backtracking can be accomplished by introducing the prefix as an
Undocumented option with a Joined argument and without Driver, Target,
Common, or any language-specific option.  Whenever we match such a fake
options prefix, we abandon further backtracking (it matches, after all),
but find_opt returns the same code it would if it hadn't found any
match, so that we resort to option mapping.

I've arranged for such remapping prefixes to not be considered when
looking for and suggesting a correct spelling for misspelled options.
While testing that, I found a few -W-started options that were not
marked as RejectNegative but should (-Wno-a, is not something we'd like
to suggest ;-)  I've also marked as such -g-started options that
it makes no sense to negate, and removed the explicit -gno- ones,
allowing their opposites to be negated.

Regstrapped on x86_64-linux-gnu and i686-linux-gnu.  Ok to install?

for  gcc/ChangeLog

* common.opt (Wa, Wl, Wp, g, gz=): Add
RejectNegative.
(gno-column-info): Remove.
(gcolumn-info): Drop RejectNegative.
(gno-): New prefix.
(gno-record-gcc-switches): Remove.
(grecord-gcc-switches): Drop RejectNegative.
(gno-split-dwarf): Remove.
(gsplit-dwarf): Drop RejectNegative.
(gno-strict-dwarf): Remove.
(gstrict-dwarf): Drop RejectNegative.
* config/darwin.opt (gfull, gused): Add RejectNegative.
* dwarf2out.c (gen_producer_string): Drop
gno-record-gcc-switches handler.
* optc-gen.awk: Add g to prefixes with negative forms.
* opts-common.c (remapping_prefix_p): New.
(find_opt): Check it.
(generate_canonical_option): Test g prefix.
(option_map): Add -gno- mapping.
(add_misspelling_candidates): Check remapping_prefix_p.

for  gcc/ada/ChangeLog

* gcc-interface/lang.opt (gant, gnatO, gnat): Add
RejectNegative.

for  gcc/c-family/ChangeLog

* c.opt (gen-decls): Add RejectNegative.
---
 gcc/ada/gcc-interface/lang.opt |6 +++---
 gcc/c-family/c.opt |2 +-
 gcc/common.opt |   38 +-
 gcc/config/darwin.opt  |4 ++--
 gcc/dwarf2out.c|1 -
 gcc/optc-gen.awk   |4 ++--
 gcc/opts-common.c  |   27 ++-
 7 files changed, 47 insertions(+), 35 deletions(-)

diff --git a/gcc/ada/gcc-interface/lang.opt b/gcc/ada/gcc-interface/lang.opt
index 241eafc..17c6dc8 100644
--- a/gcc/ada/gcc-interface/lang.opt
+++ b/gcc/ada/gcc-interface/lang.opt
@@ -81,15 +81,15 @@ Ada AdaWhy AdaSCIL
 Make \"char\" signed by default.
 
 gant
-Ada AdaWhy AdaSCIL Driver Joined Undocumented
+Ada AdaWhy AdaSCIL Driver Joined Undocumented RejectNegative
 Catch typos.
 
 gnatO
-Ada AdaWhy AdaSCIL Driver Separate
+Ada AdaWhy AdaSCIL Driver Separate RejectNegative
 Set name of output ALI file (internal switch).
 
 gnat
-Ada AdaWhy AdaSCIL Driver Joined
+Ada AdaWhy AdaSCIL Driver Joined RejectNegative
 -gnat Specify options to GNAT.
 
 fbuiltin-printf
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index e0ad3ab..55d9405 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -1802,7 +1802,7 @@ ObjC ObjC++ Var(flag_zero_link)
 Generate lazy class lookup (via objc_getClass()) for use in Zero-Link mode.
 
 gen-decls
-ObjC ObjC++ Driver Var(flag_gen_declaration)
+ObjC ObjC++ Driver Var(flag_gen_declaration) RejectNegative
 Dump declarations to a .decl file.
 
 femit-struct-debug-baseonly
diff --git a/gcc/common.opt b/gcc/common.opt
index 1cb1c83..d18559a 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -532,13 +532,13 @@ Common RejectNegative Warning Alias(Wextra)
 This switch is deprecated; 

Re: Statement Frontier Notes, Location Views, and Inlined Entry Point Markers

[Alexandre Oliva]

On Aug 23, 2017, Richard Biener  wrote:

>>> if they are not a problem up until here why care now?

>> IIRC we do have a limit for VTA notes too, but there's a C++ testcase
>> (g++.dg/tree-ssa/pr14703.C) that expands and inlines fibonacci template
>> functions so deep, more than doubling the number of statements at all
>> but the base recursion levels, so we'd end up with over 2^{85+} debug
>> stmts if we didn't cut them off somehow.

> Yeah, but I meant we've kept them throughout GIMPLE (for all functions!)
> but are dropping them here at RTL expansion (which we'll have only a
> single live RTL function at a time).  That looks odd ;)

Aah, yeah, the point is, if we find we exceeded the limit, we don't
bother to clean up the gimple, we just refrain from wasting further time
with it, which we would if we converted them to RTL (and then threw them
away), or copied them all when inlining into some other function.  We
could clean up at some point, just as we could stop emitting further
markers once the limit is reached, but it didn't seem important enough
to do so.  Should it prove to be, I guess it wouldn't be too hard to add
it to gimple verification passes that walk over all stmts.

> You're already dropping them at inlining as well so the RTL expansion
> check should be superfluous IMHO (yeah, unrolling might push it over
> the edge for example but all real issues should come from inlining).

The RTL expansion check is indeed not essential, but if we're over the
limit, we'll to throw it all away, so why bother expanding it and
carrying it through all RTL passes just to throw away at the end?  Or
should we not throw it away in this case, and make the limit apply only
to inlining?  But then, what if we inline lots of very large functions
into a single one, do we still want to use markers for that function?
That's not how I designed it, but I guess it might work that way too.


> Hmm, yeah.  I guess we'd have to have a multi-DEBUG_STMT that covers
> not only multiple markers but also multiple binds.  High GIMPLE has
> nested stmts so it might be tempting to wrap adjacent debug-stmts into
> a single one (basically make the IL walking overhead with debug stmts 
> smaller).
> Costs extra memory instead of less when compared to my idea of course.

Yeah.  I guess that's doable and it won't make gimple passes much
trickier: in most cases all that matters are the SSA uses in bind value
expressions, so as long as the update function can efficiently pick the
SSA uses from the op array, it could be a significant win.  

We may need some way to reset one specific bind given a use that is no
longer valid, which I don't immediately see how to implement efficiently
in a multi-debug pack .

Now, I spent some time trying to think of how to pack multiple debug
stmts in a way that made them also save memory.

For each packed stmt, we need at least one bit to indicate whether it's
a bind or just a marker.  Markers then need a locus, and another bit
indicating whether it's a begin stmt marker or an inline entry point
marker.  Debug bind stmts need one bit to indicate tell src binds from
regular ones, and two trees (no locus).

It is unlikely that it would make sense to allocate extra memory, be it
trees holding integral values, be it other arrays to hold them.  I'm
thinking we'd be better off storing some of these bits in an analogous
of the trailing op VLA, that would be present in gdebug but that would
deal with GGC and ssa updates in its own way.

For packs with few stmts, we could use bits from the subcode to indicate
the count and the kinds.  We could use the gimple locus for the first
marker, and then perhaps pack pairs of loci in tree pointer operands (if
their sizes are 1:2, as in lp64).

When packing more than few stmts, we could then define a format for a
32-bit word to hold the bits for an additional set of stmts, possibly
packed in the same word as a locus or another such bit pack.  Ideally,
should we need more than one of these, we should indicate upfront how
many of these there are, or at least how many ops are used.

I was thinking it would be ideal if combining two many-stmts debug stmts
could require little more than allocating a gimple with a larger ops
array and copying (most of) the original op arrays to the right places.

But...  this all feels far too hackish and not very maintainable or
forward-looking.  E.g., if we add more kinds of debug stmts, the bit
counting suddenly no longer applies, and needs to be reworked.

So I guess that's also doable, and would save some memory indeed,
but...  do you think it's worth it?


>>> Btw, just asking as I helped to get the GIMPLE FE in, did you
>>> consider adding GIMPLE FE support for the various debug stmts
>>> we then have?  First thing would be arriving at a syntax I guess.
>>> __DEBUG x = ...; for binds, __STMT; __INLINE; for the other two?
>>> Not sure how to express they encode some location though...
>>> (binds have no location, 

[pr 81982] wide-int.h change broke native arm-* gcc

[Szabolcs Nagy]

i don't see corresponding email thread in gcc-patches, but
since this commit:
https://gcc.gnu.org/viewcvs/gcc?view=revision=251260

i see arm-* native compiler miscompile libstdc++,
note that the cross compiler compiles it correctly.

i guess it can be some latent bug that got uncovered,
but it is reliably reproducible, i reported

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81982

those who run native arm gcc tests may see c++ execution tests
timeout while spinning in __cxa_guard_acquire around a futex
syscall.

i'm still debugging it, but ideas and help is welcome.

Re: [PATCH v2] Simplify pow with constant

[Wilco Dijkstra]

Jeff Law wrote:
> Right.  exp is painful in glibc, but pow is *dramatically* more painful
> and likely always will be.
>
> Siddhesh did some great work in bringing those costs down in glibc but
> the more code we can reasonably shunt into exp instead of pow, the better.
>
> It's likely pow will always be significantly more expensive than exp.
> It's also likely that predicting when these functions are going to go
> off the fast paths is painful.

With a modern implementation there won't be any slow path - it's completely
unnecessary, and you can get 100x speedup by simply doing things in a
sane way.

Szabolc's version of powf is almost literally doing exp(log(x) * y), so exp is
about twice as fast as pow.

Wilco

Re: c-family PATCH to improve -Wtautological-compare (PR c/81783)

[Marek Polacek]

Ping.

On Wed, Aug 16, 2017 at 05:24:56PM +0200, Marek Polacek wrote:
> On Wed, Aug 16, 2017 at 11:07:36AM -0400, David Malcolm wrote:
> > On Wed, 2017-08-16 at 16:29 +0200, Marek Polacek wrote:
> > > This patch improves -Wtautological-compare so that it also detects
> > > bitwise comparisons involving & and | that are always true or false,
> > > e.g.
> > > 
> > >   if ((a & 16) == 10)
> > > return 1;
> > > 
> > > can never be true.  Note that e.g. "(a & 9) == 8" is *not* always
> > > false
> > > or true.
> > > 
> > > I think it's pretty straightforward with one snag: we shouldn't warn
> > > if
> > > the constant part of the bitwise operation comes from a macro, but
> > > currently
> > > that's not possible, so I XFAILed this in the new test.
> > 
> > Maybe I'm missing something here, but why shouldn't it warn when the
> > constant comes from a macro?
> 
> Just my past experience.  Sometimes you can't really control the macro
> and then you get annoying warnings.
> 
> E.g. I had to tweak the warning that warns about if (i == i) to not warn about
>   
>   #define N 2
>   if (a[N] == a[2]) {}
> 
> because that gave bogus warning during bootstrap, if I recall well.
> 
> > At the end of your testcase you have this example:
> > 
> > #define N 0x10
> >   if ((a & N) == 10) /* { dg-bogus "bitwise comparison always evaluates to 
> > false" "" { xfail *-*-* } } */
> >  return 1;
> >   if ((a | N) == 10) /* { dg-bogus "bitwise comparison always evaluates to 
> > false" "" { xfail *-*-* } } */
> >return 1;
> > 
> > That code looks bogus to me (and if the defn of "N" is further away,
> > it's harder to spot that it's wrong): shouldn't we warn about it?
> 
> I'm glad you think so.  More than happy to make it an expected warning.
> 
> > > This has found one issue in the GCC codebase and it's a genuine bug:
> > > .  
> > 
> > In this example GOVD_WRITTEN is from an enum, not a macro, but if
> > GOVD_WRITTEN had been a macro, shouldn't we still issue a warning?
> 
> I feel like we should, but some might feel otherwise.
> 
> Thanks,
> 
>   Marek

Marek

[C++ PATCH] METHOD_VEC sorting

[Nathan Sidwell]

While I intend to kill METHOD_VEC, I noticed its comparison function 
contemplated functions with NULL DECL_NAMEs.  Those aren't a thing.


applied to trunk.

nathan
--
Nathan Sidwell
2017-08-25  Nathan Sidwell  

	* class.c (method_name_cmp, resort_method_name_cmp): Method names
	can never be NULL.

Index: class.c
===
--- class.c	(revision 251348)
+++ class.c	(working copy)
@@ -2263,12 +2263,6 @@ method_name_cmp (const void* m1_p, const
   const tree *const m1 = (const tree *) m1_p;
   const tree *const m2 = (const tree *) m2_p;
 
-  if (*m1 == NULL_TREE && *m2 == NULL_TREE)
-return 0;
-  if (*m1 == NULL_TREE)
-return -1;
-  if (*m2 == NULL_TREE)
-return 1;
   if (OVL_NAME (*m1) < OVL_NAME (*m2))
 return -1;
   return 1;
@@ -2282,20 +2276,13 @@ resort_method_name_cmp (const void* m1_p
 {
   const tree *const m1 = (const tree *) m1_p;
   const tree *const m2 = (const tree *) m2_p;
-  if (*m1 == NULL_TREE && *m2 == NULL_TREE)
-return 0;
-  if (*m1 == NULL_TREE)
+
+  tree n1 = OVL_NAME (*m1);
+  tree n2 = OVL_NAME (*m2);
+  resort_data.new_value (, resort_data.cookie);
+  resort_data.new_value (, resort_data.cookie);
+  if (n1 < n2)
 return -1;
-  if (*m2 == NULL_TREE)
-return 1;
-  {
-tree d1 = OVL_NAME (*m1);
-tree d2 = OVL_NAME (*m2);
-resort_data.new_value (, resort_data.cookie);
-resort_data.new_value (, resort_data.cookie);
-if (d1 < d2)
-  return -1;
-  }
   return 1;
 }
 

[C++ PATCH] Find conv ops by name

[Nathan Sidwell]

Conversion operators squirrel the target type on their IDENTIFIER_TYPE, 
thus have to have distinct IDENTIFIER_NODEs.  That doesn't work with 
find-by-name, and hence they currently have a unique slot in the 
METHOD_VEC -- yesterday's patch put them all on the same slot.


This patch allows them to be found by name.  I add conv_op_identifier, a 
unique identifier_node, and create conv_op_marker a unique and bogus 
function_decl.  The lookup machinery prepends an overload node pointing 
at this marker function to the set of conversion operators.


Thus we can now find them by name.  Either conv_op_identifier to get all 
the overloads.  Or by their unique conv-op identifiers to get just the 
conversion operators to that particular type.  In both cases the marker 
function is stripped off, so it is never seen.  The latter case works by 
post-processing the located overload set pulling out just those (usually 
at most one) converting the the required type.


And with that CLASSTYPE_FIRST_CONVERSION_SLOT is no more.  Applied to trunk.

nathan
--
Nathan Sidwell
2017-08-25  Nathan Sidwell  

	Conversion operators have a special name
	* cp-tree.h (CPTI_CONV_OP_MARKER, CPTI_CONV_OP_IDENTIFIER): New.
	(conv_op_marker, conv_op_identifier): New.
	(CLASSTYPE_FIRST_CONVERSION_SLOT): Delete.
	* decl.c (initialize_predefined_identifiers): Add
	conv_op_identifier.
	(cxx_init_decl_processing): Create conv_op_marker.
	* decl2.c (check_classfn): Lookup conv-ops by name.
	* class.c (add_method): Use conv_op_identifier & conv_op_marker.
	(resort_type_method_vec): Don't skip conv-ops.
	(finish_struct_methods, warn_hidden): Likewise.
	* name-lookup.h (lookup_all_conversions): Delete.
	* name-lookup.c (lookup_conversion_operator): Replace with ...
	(extract_conversion_operator): ... this.
	(lookup_fnfields_slot_nolazy): Find conv-ops by name.
	(lookup_all_conversions): Delete.
	* pt.c (check_explicit_specialization): Find conv-ops by name.
	* search.c (lookup_conversions_r): Likewise.

Index: class.c
===
--- class.c	(revision 251340)
+++ class.c	(working copy)
@@ -1017,9 +1017,6 @@ add_method (tree type, tree method, bool
   if (method == error_mark_node)
 return false;
 
-  bool complete_p = COMPLETE_TYPE_P (type);
-  bool conv_p = DECL_CONV_FN_P (method);
-
   vec *method_vec = CLASSTYPE_METHOD_VEC (type);
   if (!method_vec)
 {
@@ -1032,32 +1029,45 @@ add_method (tree type, tree method, bool
   grok_special_member_properties (method);
 
   bool insert_p = true;
-  unsigned slot;
-  tree m;
+  tree method_name = DECL_NAME (method);
+  bool complete_p = COMPLETE_TYPE_P (type);
+  bool conv_p = IDENTIFIER_CONV_OP_P (method_name);
+
+  if (conv_p)
+method_name = conv_op_identifier;
 
   /* See if we already have an entry with this name.  */
-  for (slot = CLASSTYPE_FIRST_CONVERSION_SLOT;
-   vec_safe_iterate (method_vec, slot, );
-   ++slot)
+  unsigned slot;
+  tree m;
+  for (slot = 0; vec_safe_iterate (method_vec, slot, ); ++slot)
 {
-  m = OVL_FIRST (m);
-  if (conv_p)
-	{
-	  if (DECL_CONV_FN_P (m))
-	insert_p = false;
-	  break;
-	}
-  if (DECL_NAME (m) == DECL_NAME (method))
+  m = DECL_NAME (OVL_FIRST (m));
+  if (m == method_name)
 	{
 	  insert_p = false;
 	  break;
 	}
-  if (complete_p
-	  && !DECL_CONV_FN_P (m)
-	  && DECL_NAME (m) > DECL_NAME (method))
+  if (complete_p && m > method_name)
 	break;
 }
   tree current_fns = insert_p ? NULL_TREE : (*method_vec)[slot];
+
+  tree conv_marker = NULL_TREE;
+  if (conv_p)
+{
+  /* For conversion operators, we prepend a dummy overload
+	 pointing at conv_op_marker.  That function's DECL_NAME is
+	 conv_op_identifier, so we can use identifier equality to
+	 locate it.  */
+  if (current_fns)
+	{
+	  gcc_checking_assert (OVL_FUNCTION (current_fns) == conv_op_marker);
+	  conv_marker = current_fns;
+	  current_fns = OVL_CHAIN (current_fns);
+	}
+  else
+	conv_marker = ovl_make (conv_op_marker, NULL_TREE);
+}
   gcc_assert (!DECL_EXTERN_C_P (method));
 
   /* Check to see if we've already got this method.  */
@@ -1206,7 +1216,12 @@ add_method (tree type, tree method, bool
   current_fns = ovl_insert (method, current_fns, via_using);
 
   if (conv_p)
-TYPE_HAS_CONVERSION (type) = 1;
+{
+  TYPE_HAS_CONVERSION (type) = 1;
+  /* Prepend the marker function.  */
+  OVL_CHAIN (conv_marker) = current_fns;
+  current_fns = conv_marker;
+}
   else if (!complete_p && !IDENTIFIER_CDTOR_P (DECL_NAME (method)))
 push_class_level_binding (DECL_NAME (method), current_fns);
 
@@ -2294,23 +2309,10 @@ resort_type_method_vec (void* obj,
 {
   if (vec *method_vec = (vec *) obj)
 {
-  int len = method_vec->length ();
-  int slot;
-
-  /* The type conversion ops have to live at the front of the vec, so we
-	 can't sort them.  */
-  for (slot = 

Re: [PATCH, GCC/ARM] Remove ARMv8-M code for D17-D31

[Thomas Preudhomme]

Hi,

I've now also added a couple more changes:

* size to_clear_bitmap according to maxregno to be consistent with its use
* use directly TARGET_HARD_FLOAT instead of clear_vfpregs


Original message below (ChangeLog unchanged):

Function cmse_nonsecure_entry_clear_before_return has code to deal with
high VFP register (D16-D31) while ARMv8-M Baseline and Mainline both do
not support more than 16 double VFP registers (D0-D15). This makes this
security-sensitive code harder to read for not much benefit since
libcall for cmse_nonsecure_call functions do not deal with those high
VFP registers anyway.

This commit gets rid of this code for simplicity and fixes 2 issues in
the same function:

- stop the first loop when reaching maxregno to avoid dealing with VFP
  registers if targetting Thumb-1 or using -mfloat-abi=soft
- include maxregno in that loop

ChangeLog entry is as follows:

*** gcc/ChangeLog ***

2017-06-13  Thomas Preud'homme  

* config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
Extensions with more than 16 double VFP registers.
(cmse_nonsecure_entry_clear_before_return): Remove second entry of
to_clear_mask and all code related to it.  Replace the remaining
entry by a sbitmap and adapt code accordingly.

Testing: Testsuite shows no regression when run for ARMv8-M Baseline and
ARMv8-M Mainline.

Is this ok for trunk?

Best regards,

Thomas

On 23/08/17 11:56, Thomas Preudhomme wrote:

Ping?

Best regards,

Thomas

On 17/07/17 17:25, Thomas Preudhomme wrote:
My bad, found an off-by-one error in the sizing of bitmaps. Please find fixed 
patch in attachment.


ChangeLog entry is unchanged:

*** gcc/ChangeLog ***

2017-06-13  Thomas Preud'homme  

 * config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
 Extensions with more than 16 double VFP registers.
 (cmse_nonsecure_entry_clear_before_return): Remove second entry of
 to_clear_mask and all code related to it.  Replace the remaining
 entry by a sbitmap and adapt code accordingly.

Best regards,

Thomas

On 17/07/17 09:52, Thomas Preudhomme wrote:

Ping?

Best regards,

Thomas

On 12/07/17 09:59, Thomas Preudhomme wrote:

Hi Richard,

On 07/07/17 15:19, Richard Earnshaw (lists) wrote:


Hmm, I think that's because really this is a partial conversion.  It
looks like doing this properly would involve moving that existing code
to use sbitmaps as well.  I think doing that would be better for
long-term maintenance perspectives, but I'm not going to insist that you
do it now.


There's also the assert later but I've found a way to improve it slightly. 
While switching to auto_sbitmap I also changed the code slightly to allocate 
directly bitmaps to the right size. Since the change is probably bigger than 
what you had in mind I'd appreciate if you can give me an OK again. See 
updated patch in attachment. ChangeLog entry is unchanged:


2017-06-13  Thomas Preud'homme  

 * config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
 Extensions with more than 16 double VFP registers.
 (cmse_nonsecure_entry_clear_before_return): Remove second entry of
 to_clear_mask and all code related to it.  Replace the remaining
 entry by a sbitmap and adapt code accordingly.



As a result I'll let you take the call as to whether you keep this
version or go back to your earlier patch.  If you do decide to keep this
version, then see the comment below.


Given the changes I'm more happy with how the patch looks now and making it 
go in can be a nice incentive to change other ARMv8-M Security Extension 
related code later on.


Best regards,

Thomas
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 3d15a8185a74164743961d7d666cef4d60b8b11e..680a3c564bdad4ae7cacd57b61f099bdf42d3e73 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -3658,6 +3658,11 @@ arm_option_override (void)
   if (use_cmse && !arm_arch_cmse)
 error ("target CPU does not support ARMv8-M Security Extensions");
 
+  /* We don't clear D16-D31 VFP registers for cmse_nonsecure_call functions
+ and ARMv8-M Baseline and Mainline do not allow such configuration.  */
+  if (use_cmse && LAST_VFP_REGNUM > LAST_LO_VFP_REGNUM)
+error ("ARMv8-M Security Extensions incompatible with selected FPU");
+
   /* Disable scheduling fusion by default if it's not armv7 processor
  or doesn't prefer ldrd/strd.  */
   if (flag_schedule_fusion == 2
@@ -25038,42 +25043,37 @@ thumb1_expand_prologue (void)
 void
 cmse_nonsecure_entry_clear_before_return (void)
 {
-  uint64_t to_clear_mask[2];
+  int regno, maxregno = TARGET_HARD_FLOAT ? LAST_VFP_REGNUM : IP_REGNUM;
   uint32_t padding_bits_to_clear = 0;
   uint32_t * padding_bits_to_clear_ptr = _bits_to_clear;
-  int regno, maxregno = IP_REGNUM;
+  auto_sbitmap to_clear_bitmap (maxregno + 1);
   tree result_type;
   rtx result_rtl;
 
-  to_clear_mask[0] = 

Re: [PATCH 0/2] backport c++ header fixes to gcc-5-branch

[Jack Howarth]

Szabolcs,
 Can you help get these back ports into gcc-5-branch?

https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00478.html
https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00477.html

Thanks in advance.
 Jack

On Mon, Aug 7, 2017 at 1:12 AM, Ryan Mounce  wrote:
> Fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81037
>
> Bootstrap now succeeds using Xcode 9 toolchain.
>
> Tested on macOS 10.13 beta, however same issue reported on macOS 10.12
> with Xcode 9.
>
> Ryan Mounce (2):
>   (header usage fix) remove unused system header includes
>   (header usage fix) include c++ headers in system.h
>
>  gcc/ChangeLog| 29 +
>  gcc/auto-profile.c   |  6 ++
>  gcc/c/c-objc-common.c|  2 --
>  gcc/config/sh/sh.c   |  2 +-
>  gcc/config/sh/sh_treg_combine.cc |  7 +++
>  gcc/cp/error.c   |  2 --
>  gcc/diagnostic.c |  2 --
>  gcc/fortran/error.c  |  2 --
>  gcc/fortran/trans-common.c   |  2 +-
>  gcc/genmatch.c   |  1 -
>  gcc/graphite-isl-ast-to-gimple.c |  2 +-
>  gcc/ipa-icf-gimple.c |  2 +-
>  gcc/ipa-icf.c|  2 +-
>  gcc/pretty-print.c   |  2 --
>  gcc/system.h | 12 
>  gcc/toplev.c |  2 --
>  16 files changed, 51 insertions(+), 26 deletions(-)
>
> --
> 2.13.2 (Apple Git-90)
>

Re: [PATCH] Enable libitm DLL build on Cygwin/Mingw

[JonY]

On 08/09/2017 01:02 PM, JonY wrote:
> Fixes libtool calls in libitm. Patch OK for trunk?
> 
> 2017-08-09  Jonathan Yong  <10wa...@gmail.com>
> 
>   * configure.ac: Check libtool flags.
>   * Makefile.am: Use lt_host_flags.
>   * configure: Regenerated.
>   * Makefile.in: Regenerated.
>   * testsuite/Makefile.in: Regenerated.
> 
> 
> Index: configure.ac
> ===
> --- configure.ac  (revision 250986)
> +++ configure.ac  (working copy)
> @@ -147,6 +147,7 @@
>  
>  # Configure libtool
>  AM_PROG_LIBTOOL
> +ACX_LT_HOST_FLAGS
>  AC_SUBST(enable_shared)
>  AC_SUBST(enable_static)
>  
> Index: Makefile.am
> ===
> --- Makefile.am   (revision 250986)
> +++ Makefile.am   (working copy)
> @@ -54,7 +54,7 @@
>  # want or need libstdc++.
>  libitm_la_DEPENDENCIES = $(libitm_version_dep)
>  libitm_la_LINK = $(LINK) $(libitm_la_LDFLAGS)
> -libitm_la_LDFLAGS = $(libitm_version_info) $(libitm_version_script)
> +libitm_la_LDFLAGS = $(libitm_version_info) $(libitm_version_script) 
> $(lt_host_flags)
>  
>  libitm_la_SOURCES = \
>   aatree.cc alloc.cc alloc_c.cc alloc_cpp.cc barrier.cc beginend.cc \
> 

Ping.



signature.asc
Description: OpenPGP digital signature

[PATCH] libstdc++: test for copy_n/istreambuf_iterator

[Petr Ovtchenkov]

copy_n return result + n (i.e. increment OutputIterator n times) and
increment InputIterator max(0, n - 1).

This is issue 81857.

See also https://cplusplus.github.io/LWG/issue2471
---
 .../testsuite/25_algorithms/copy_n/81857.cc| 83 ++
 1 file changed, 83 insertions(+)
 create mode 100644 libstdc++-v3/testsuite/25_algorithms/copy_n/81857.cc

diff --git a/libstdc++-v3/testsuite/25_algorithms/copy_n/81857.cc 
b/libstdc++-v3/testsuite/25_algorithms/copy_n/81857.cc
new file mode 100644
index 000..adb6c35
--- /dev/null
+++ b/libstdc++-v3/testsuite/25_algorithms/copy_n/81857.cc
@@ -0,0 +1,83 @@
+// { dg-options "-std=gnu++11" }
+
+// Copyright (C) 2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// .
+
+#include 
+#include 
+#include 
+#include 
+
+// libstdc++/81857
+void test01()
+{
+  using namespace std;
+  bool test __attribute__((unused)) = true;
+
+  std::stringstream s;
+  char b[] = "c2ee3d09-43b3-466d-b490-db35999a22cf";
+  char r[] = "";
+  //  012345678901234567890123456789012345
+  //  0 1 2 3
+  s << b;
+  VERIFY( !s.fail() );
+
+  VERIFY( s.tellg() == 0 );
+  /*
+https://cplusplus.github.io/LWG/issue2471
+
+It's unspecified how many times copy_n increments the InputIterator.
+uninitialized_copy_n is specified to increment it exactly n times,
+which means if an istream_iterator is used then the next character
+after those copied is read from the stream and then discarded, losing data.
+
+I believe all three of Dinkumware, libc++ and libstdc++ implement copy_n
+with n - 1 increments of the InputIterator, which avoids reading and
+discarding a character when used with istream_iterator, but is inconsistent
+with uninitialized_copy_n and causes surprising behaviour with
+istreambuf_iterator instead, because copy_n(in, 2, copy_n(in, 2, out))
+is not equivalent to copy_n(in, 4, out)
+   */
+
+  /*
+copy_n return result + n (i.e. increment OutputIterator n times) and
+increment InputIterator max(0, n - 1).
+   */
+  std::copy_n( std::istreambuf_iterator(s), 36, r );
+  VERIFY( !s.fail() );
+  VERIFY( memcmp(b, r, 36) == 0 );
+
+  char c = 'q';
+  std::copy_n( std::istreambuf_iterator(s), 1,  );
+  VERIFY( std::istreambuf_iterator(s) != 
std::istreambuf_iterator() ); // see comment above
+  VERIFY( !s.fail() ); // surprise, see comment above
+  VERIFY( c != 'q' );
+  VERIFY( c == 'f' ); // surprise, see comment above
+
+  // Suggested workaround technique:
+  std::istreambuf_iterator iis(s);
+  ++iis; // <---
+  std::copy_n( iis, 1,  );
+  // VERIFY( s.fail() ); // ?!
+  VERIFY( iis == std::istreambuf_iterator() );
+}
+
+int main()
+{
+  test01();
+  return 0;
+}
-- 
2.10.1

Re: [PATCH] handle pathological anti-ranges in gimple_fold_builtin_memory_op (PR 81908)

[Richard Sandiford]

Martin Sebor  writes:
> On 08/24/2017 08:03 AM, Richard Biener wrote:
>> On Wed, Aug 23, 2017 at 9:42 PM, Martin Sebor  wrote:
>>> Bug 81908 is about a -Wstringop-overflow warning for a Fortran
>>> test triggered by a recent VRP improvement.  A simple test case
>>> that approximates the warning is:
>>>
>>>   void f (char *d, const char *s, size_t n)
>>>   {
>>> if (n > 0 && n <= SIZE_MAX / 2)
>>>   n = 0;
>>>
>>> memcpy (d, s, n);   // n in ~[1, SIZE_MAX / 2]
>>>   }
>>>
>>> Since the only valid value of n is zero the call to memcpy can
>>> be considered a no-op (a value of n > SIZE_MAX is in excess of
>>> the maximum size of the largest object and would surely make
>>> the call crash).
>>>
>>> The important difference between the test case and Bug 81908
>>> is that in the latter, the code is emitted by GCC itself from
>>> what appears to be correct source (looks like it's the result
>>> of the loop distribution pass).  I believe the warning for
>>> the test case above and for other human-written code like it
>>> is helpful, but warning for code emitted by GCC, even if it's
>>> dead or unreachable, is obviously not (at least not to users).
>>>
>>> The attached patch enhances the gimple_fold_builtin_memory_op
>>> function to eliminate this patholohgical case by making use
>>> of range information to fold into no-ops calls to memcpy whose
>>> size argument is in a range where the only valid value is zero.
>>> This gets rid of the warning and improves the emitted code.
>>>
>>> Tested on x86_64-linux.
>>
>> @@ -646,6 +677,12 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterator 
>> *gsi,
>>tree destvar, srcvar;
>>location_t loc = gimple_location (stmt);
>>
>> +  if (tree valid_len = only_valid_value (len))
>> +{
>> +  /* LEN is in range whose only valid value is zero.  */
>> +  len = valid_len;
>> +}
>> +
>>/* If the LEN parameter is zero, return DEST.  */
>>if (integer_zerop (len))
>>
>> just enhance this check to
>>
>>   if (integer_zerop (len)
>>   || size_must_be_zero_p (len))
>>
>> ?  'only_valid_value' looks too generic for this.
>
> Sure.
>
> FWIW, the reason I did it this was because my original patch
> returned error_mark_node for entirely invalid ranges and had
> the caller replace the call with a trap.  I decided not to
> include that part in this fix to keep it contained.
>
>>
>> +  if (!wi::fits_uhwi_p (min) || !wi::fits_uhwi_p (max))
>> +return NULL_TREE;
>> +
>>
>> why?
>
> Only because I never remember what APIs are safe to use with
> what input.
>
>> +  if (wi::eq_p (min, wone)
>> +  && wi::geu_p (max + 1, ssize_max))
>>
>>if (wi::eq_p (min, 1)
>>   && wi::gtu_p (max, wi::max_value (prec, SIGNED)))
>>
>> your ssize_max isn't signed size max, and max + 1 might overflow to zero.
>
> You're right that the addition to max would be better done
> as subtraction from the result of (1 << N).  Thank you.
>
> If (max + 1) overflowed then (max == TYPE_MAX) would have
> to hold which I thought could never be true for an anti
> range. (The patch includes tests for this case.)  Was I
> wrong?
>
> Attached is an updated version with the suggested changes
> plus an additional test to verify the absence of warnings.
>
> Martin
>
> PR middle-end/81908 - FAIL: gfortran.dg/alloc_comp_auto_array_2.f90 -O3 -g 
> -m32
>
> gcc/ChangeLog:
>
>   PR middle-end/81908
>   * gimple-fold.c (size_must_be_zero_p): New function.
>   (gimple_fold_builtin_memory_op): Call it.
>
> gcc/testsuite/ChangeLog:
>
>   PR middle-end/81908
>   * gcc.dg/tree-ssa/builtins-folding-gimple-2.c: New test.
>   * gcc.dg/tree-ssa/builtins-folding-gimple-3.c: New test.
>   * gcc.dg/tree-ssa/pr81908.c: New test.
>
> diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
> index 251446c..c4184fa 100644
> --- a/gcc/gimple-fold.c
> +++ b/gcc/gimple-fold.c
> @@ -628,6 +628,36 @@ var_decl_component_p (tree var)
>return SSA_VAR_P (inner);
>  }
>  
> +/* If the SIZE argument representing the size of an object is in a range
> +   of values of which exactly one is valid (and that is zero), return
> +   true, otherwise false.  */
> +
> +static bool
> +size_must_be_zero_p (tree size)
> +{
> +  if (integer_zerop (size))
> +return true;
> +
> +  if (TREE_CODE (size) != SSA_NAME)
> +return false;
> +
> +  wide_int min, max;
> +  enum value_range_type rtype = get_range_info (size, , );
> +  if (rtype != VR_ANTI_RANGE)
> +return false;
> +
> +  tree type = TREE_TYPE (size);
> +  int prec = TYPE_PRECISION (type);
> +
> +  wide_int wone = wi::one (prec);
> +
> +  /* Compute the value of SSIZE_MAX, the largest positive value that
> + can be stored in ssize_t, the signed counterpart of size_t .  */
> +  wide_int ssize_max = wi::lshift (wi::one (prec), prec - 1) - 1;
> +
> +  return wi::eq_p (min, wone) && wi::geu_p (max, ssize_max);

Just a stylistic thing, but since the only use of "wone" is in
the eq_p, it'd be simpler just 

Re: [PATCH GCC][04/06]Add copying interface for dependence_info

[Richard Biener]

On August 25, 2017 12:25:57 AM GMT+02:00, Jeff Law  wrote:
>On 08/14/2017 03:19 AM, Bin Cheng wrote:
>> HI,
>> This patch adds copying interface for dependence_info.  The
>methodology
>> is we don't copy such information by default, and this interface
>should
>> be called explicitly when it is safe and necessary to do so.  Just
>like
>> this patch uses the interface in ivopts.
>> Bootstrap and test in series.  Is it OK?
>> 
>> Thanks,
>> bin
>> 2017-08-10  Bin Cheng  
>> 
>>  * tree-ssa-address.c (copy_dependence_info): New function.
>>  * tree-ssa-address.h (copy_dependence_info): New declaration.
>>  * tree-ssa-loop-ivopts.c (rewrite_use_address): Call above func.
>So do we have any structure sharing assumptions on the alias
>structures?
>ie, are we setting up the possibility that these objects will be shared
>and that someone will modify them in a way that works in one context,
>but not another?
>
>If they're readonly after creation, then obviously this isn't a
>concern.
>
>I wouldn't consider this an object or an ACK for the patch at this
>point. More a design question we need to answer.

Note there are existing places where we copy the info. Note that for example 
the inliner re-maps the cliques during copying to not introduce false 
non-dependennces
This is another possibility to avoid the issue with unrolling. 

Richard. 

>Jeff

Re: [PATCH] handle pathological anti-ranges in gimple_fold_builtin_memory_op (PR 81908)

[Richard Biener]

On August 24, 2017 11:52:41 PM GMT+02:00, Jeff Law  wrote:
>On 08/24/2017 09:03 AM, Martin Sebor wrote:
>> On 08/24/2017 08:03 AM, Richard Biener wrote:
>>> On Wed, Aug 23, 2017 at 9:42 PM, Martin Sebor 
>wrote:
 Bug 81908 is about a -Wstringop-overflow warning for a Fortran
 test triggered by a recent VRP improvement.  A simple test case
 that approximates the warning is:

   void f (char *d, const char *s, size_t n)
   {
 if (n > 0 && n <= SIZE_MAX / 2)
   n = 0;

 memcpy (d, s, n);   // n in ~[1, SIZE_MAX / 2]
   }

 Since the only valid value of n is zero the call to memcpy can
 be considered a no-op (a value of n > SIZE_MAX is in excess of
 the maximum size of the largest object and would surely make
 the call crash).

 The important difference between the test case and Bug 81908
 is that in the latter, the code is emitted by GCC itself from
 what appears to be correct source (looks like it's the result
 of the loop distribution pass).  I believe the warning for
 the test case above and for other human-written code like it
 is helpful, but warning for code emitted by GCC, even if it's
 dead or unreachable, is obviously not (at least not to users).

 The attached patch enhances the gimple_fold_builtin_memory_op
 function to eliminate this patholohgical case by making use
 of range information to fold into no-ops calls to memcpy whose
 size argument is in a range where the only valid value is zero.
 This gets rid of the warning and improves the emitted code.

 Tested on x86_64-linux.
>>>
>>> @@ -646,6 +677,12 @@ gimple_fold_builtin_memory_op
>>> (gimple_stmt_iterator *gsi,
>>>tree destvar, srcvar;
>>>location_t loc = gimple_location (stmt);
>>>
>>> +  if (tree valid_len = only_valid_value (len))
>>> +{
>>> +  /* LEN is in range whose only valid value is zero.  */
>>> +  len = valid_len;
>>> +}
>>> +
>>>/* If the LEN parameter is zero, return DEST.  */
>>>if (integer_zerop (len))
>>>
>>> just enhance this check to
>>>
>>>   if (integer_zerop (len)
>>>   || size_must_be_zero_p (len))
>>>
>>> ?  'only_valid_value' looks too generic for this.
>> 
>> Sure.
>> 
>> FWIW, the reason I did it this was because my original patch
>> returned error_mark_node for entirely invalid ranges and had
>> the caller replace the call with a trap.  I decided not to
>> include that part in this fix to keep it contained.
>Seems reasonable.  Though I would suggest going forward with trap
>replacement for clearly invalid ranges as a follow-up.  Once you do
>trap
>replacement, the input operands all become dead as does all the code
>after the trap and the outgoing edges in the CFG.
>
>That often exposes a fair amount of cleanup.  Furthermore on targets
>that have conditional traps, the controlling condition often turns into
>a conditional trap.  In all, you get a lot of nice cascading effects
>when you turn something that is clearly bogus into a trap.
>
>gimple-ssa-isolate-erroneous-paths probably has the infrastructure you
>need, including a code you can crib to detect PHI arguments which would
>cause bogus behavior and allow you to isolate that specific path.
>
>
>
>> 
>>>
>>> +  if (!wi::fits_uhwi_p (min) || !wi::fits_uhwi_p (max))
>>> +return NULL_TREE;
>>> +
>>>
>>> why?
>> 
>> Only because I never remember what APIs are safe to use with
>> what input.
>> 
>>> +  if (wi::eq_p (min, wone)
>>> +  && wi::geu_p (max + 1, ssize_max))
>>>
>>>if (wi::eq_p (min, 1)
>>>   && wi::gtu_p (max, wi::max_value (prec, SIGNED)))
>>>
>>> your ssize_max isn't signed size max, and max + 1 might overflow to
>zero.
>> 
>> You're right that the addition to max would be better done
>> as subtraction from the result of (1 << N).  Thank you.
>> 
>> If (max + 1) overflowed then (max == TYPE_MAX) would have
>> to hold which I thought could never be true for an anti
>> range. (The patch includes tests for this case.)  Was I
>> wrong?
>Couldn't we have an anti range like ~[TYPE_MAX,TYPE_MAX]?  Or am I
>misunderstanding something.
>
>> 
>> Attached is an updated version with the suggested changes
>> plus an additional test to verify the absence of warnings.
>The patch is OK.
>
>I'll note this is another use of anti-ranges.  I'd really like to see
>Aldy's work on the new range API move forward and get rid of
>anti-ranges.

Note that anti-ranges are not complicated to handle. In fact the current API 
could be trivially extended to return two ranges for them. The complication 
will be in the callers which will have to handle the unioning.

I'd like to get rid of anti-ranges in VRP and mainly for the reason that 
handling unioning of N ranges will increase precision. I do not see too many 
uses of the increased precision outside of VRP which is where Aldhys work is 
solely residing. 

Richard. 

>THanks,
>jeff