public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH/expand] PR64011 Adjust bitsize when partial overflow happen for big-endian
@ 2014-12-30 10:41 Jiong Wang
  2015-01-09  5:46 ` Jeff Law
  0 siblings, 1 reply; 13+ messages in thread
From: Jiong Wang @ 2014-12-30 10:41 UTC (permalink / raw)
  To: gcc-patches

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

PR64011 is actually a general problem on all target support bit insertion
instructions.

we overflow check at the start of store_bit_field_1, but that only check the
situation where the field lies completely outside the register, while there do
have situation where the field lies partly in the register, we need to adjust
bitsize for this partial overflow situation. Without this fix, pr48335-2.c on
big-endian will broken on those arch support bit insert instruction, like arm, aarch64.

the testcase is just pr48335-2.c, before this patch is will ICE on arm and =
generate
invalid assembly on AArch64. after this patch, problem gone away.

ok for trunk?

bootstrap OK on x86-64 && aarch64.
no regression on x86-64

thanks.

gcc/
    PR64011
    * expmed.c (store_bit_field_using_insv): Adjust bitsize when there is partial overflow.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fix-expmed-partial-overflow.patch --]
[-- Type: text/x-patch; name=fix-expmed-partial-overflow.patch, Size: 1000 bytes --]

diff --git a/gcc/expmed.c b/gcc/expmed.c
index 0304e46..61aec39 100644
--- a/gcc/expmed.c
+++ b/gcc/expmed.c
@@ -535,6 +535,16 @@ store_bit_field_using_insv (const extraction_insn *insv, rtx op0,
       copy_back = true;
     }
 
+  /* There are similar overflow check at the start of store_bit_field_1,
+     but that only check the situation where the field lies completely
+     outside the register, while there do have situation where the field
+     lies partialy in the register, we need to adjust bitsize for this
+     partial overflow situation.  Without this fix, pr48335-2.c on big-endian
+     will broken on those arch support bit insert instruction, like arm, aarch64
+     etc.  */
+  if (bitsize + bitnum > unit && bitnum < unit)
+    bitsize = unit - bitnum;
+
   /* If BITS_BIG_ENDIAN is zero on a BYTES_BIG_ENDIAN machine, we count
      "backwards" from the size of the unit we are inserting into.
      Otherwise, we count bits from the most significant on a

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH/expand] PR64011 Adjust bitsize when partial overflow happen for big-endian
  2014-12-30 10:41 [PATCH/expand] PR64011 Adjust bitsize when partial overflow happen for big-endian Jiong Wang
@ 2015-01-09  5:46 ` Jeff Law
  2015-01-09 13:40   ` Jiong Wang
  0 siblings, 1 reply; 13+ messages in thread
From: Jeff Law @ 2015-01-09  5:46 UTC (permalink / raw)
  To: Jiong Wang, gcc-patches

On 12/30/14 03:21, Jiong Wang wrote:
> PR64011 is actually a general problem on all target support bit insertion
> instructions.
>
> we overflow check at the start of store_bit_field_1, but that only check
> the
> situation where the field lies completely outside the register, while
> there do
> have situation where the field lies partly in the register, we need to
> adjust
> bitsize for this partial overflow situation. Without this fix,
> pr48335-2.c on
> big-endian will broken on those arch support bit insert instruction,
> like arm, aarch64.
>
> the testcase is just pr48335-2.c, before this patch is will ICE on arm
> and =
> generate
> invalid assembly on AArch64. after this patch, problem gone away.
>
> ok for trunk?
>
> bootstrap OK on x86-64 && aarch64.
> no regression on x86-64
>
> thanks.
>
> gcc/
>     PR64011
>     * expmed.c (store_bit_field_using_insv): Adjust bitsize when there
> is partial overflow.
Why adjust here the size of the stored field?  Doesn't this end up 
storing less information?

If those bits are still within the object, even if the object is by some 
means living in a mixture of registers and memory, then don't we need to 
set them all?

If those bits are outside the object, then isn't the source simply 
broken because it's writing data outside the bounds of the given object?

Am I  missing something here?

jeff

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH/expand] PR64011 Adjust bitsize when partial overflow happen for big-endian
  2015-01-09  5:46 ` Jeff Law
@ 2015-01-09 13:40   ` Jiong Wang
  2015-01-09 20:12     ` Jiong Wang
  2015-01-13 22:17     ` Jeff Law
  0 siblings, 2 replies; 13+ messages in thread
From: Jiong Wang @ 2015-01-09 13:40 UTC (permalink / raw)
  To: Jeff Law, gcc-patches


On 09/01/15 05:46, Jeff Law wrote:
> On 12/30/14 03:21, Jiong Wang wrote:
>> PR64011 is actually a general problem on all target support bit insertion
>> instructions.
>>
>> we overflow check at the start of store_bit_field_1, but that only check
>> the
>> situation where the field lies completely outside the register, while
>> there do
>> have situation where the field lies partly in the register, we need to
>> adjust
>> bitsize for this partial overflow situation. Without this fix,
>> pr48335-2.c on
>> big-endian will broken on those arch support bit insert instruction,
>> like arm, aarch64.
>>
>> the testcase is just pr48335-2.c, before this patch is will ICE on arm
>> and =
>> generate
>> invalid assembly on AArch64. after this patch, problem gone away.
>>
>> ok for trunk?
>>
>> bootstrap OK on x86-64 && aarch64.
>> no regression on x86-64
>>
>> thanks.
>>
>> gcc/
>>      PR64011
>>      * expmed.c (store_bit_field_using_insv): Adjust bitsize when there
>> is partial overflow.
> Why adjust here the size of the stored field?  Doesn't this end up
> storing less information?
>
> If those bits are still within the object, even if the object is by some
> means living in a mixture of registers and memory, then don't we need to
> set them all?
>
> If those bits are outside the object, then isn't the source simply
> broken because it's writing data outside the bounds of the given object?
>
> Am I  missing something here?

Jeff,

   thanks for review and the questions.

the bug testcase is
===================

typedef short U __attribute__((may_alias, aligned (1)));
                                                                                                                       
struct S
{
   _Complex float d __attribute__((aligned (8)));
};
                                                                                                                       
void bar(struct S);
                                                                                                                       
void f5 (int x)
{
   struct S s;
   ((U *)((char *) &s.d + 1))[3] = x;
   bar (s);
}

and the final tree is:
======================
;; Function f5 (f5, funcdef_no=0, decl_uid=2608, cgraph_uid=0, symbol_order=0)
                                                                                                                       
f5 (int x)
{
   struct S s;
   short int _2;
                                                                                                                       
   <bb 2>:
   _2 = (short int) x_1(D);
   MEM[(U * {ref-all})&s + 7B] = _2; <-- A
   bar (s);
   s ={v} {CLOBBER};
   return;
                                                                                                                       
}

during expand_used_vars, gcc decide that "s" is OK reside in register pair
(regLow, regHigh), instead of on stack.

thus later, MEM[(U * {ref-all})&s + 7B] expanded into:

   regHigh + 3B which means regHigh[31:24],

so "MEM[(U * {ref-all})&s + 7B] = _2;" expanded into

   regHigh[31:24] = _2

then, store_bit_field_1 will get a to_rtx be regHigh, bitsize be 16, and bitnum be 24, so the
8bit outside of regB is safe to be ignored.

I think if store_bit_field_using_insv is called with op0 be a REG_P, and bitsize + bitnum
overflow the reg size, then it's caused by a memory object optimized into register
object.

As the outer code decided it's OK to fit the mem obj into a register, all those bit out of
reg size should be safe to ignore.

the following code in store_bit_field_using_insv haven't consider above MEM->REG situation,
it always assume bitnum + bitsize is within unit which is wrong.
  
   if (BITS_BIG_ENDIAN != BYTES_BIG_ENDIAN)
     bitnum = unit - bitsize - bitnum;

while my patch do have a problem, I should restrict the check on REG_P/SUBREG_P (op0) only.
so is the patch OK with on extra check

   if (REG_P (xop0) || (SUBREG_P (xop0) && REG_P (SUBREG_REG (xop0))))

thanks

Regards,
Jiong

>
> jeff
>
>
>


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH/expand] PR64011 Adjust bitsize when partial overflow happen for big-endian
  2015-01-09 13:40   ` Jiong Wang
@ 2015-01-09 20:12     ` Jiong Wang
  2015-01-13 22:17     ` Jeff Law
  1 sibling, 0 replies; 13+ messages in thread
From: Jiong Wang @ 2015-01-09 20:12 UTC (permalink / raw)
  To: Jiong Wang; +Cc: Jeff Law, gcc-patches

2015-01-09 13:39 GMT+00:00 Jiong Wang <jiong.wang@arm.com>:
>
> the following code in store_bit_field_using_insv haven't consider above
> MEM->REG situation,
> it always assume bitnum + bitsize is within unit which is wrong.
>    if (BITS_BIG_ENDIAN != BYTES_BIG_ENDIAN)
>     bitnum = unit - bitsize - bitnum;
>
> while my patch do have a problem, I should restrict the check on
> REG_P/SUBREG_P (op0) only.
> so is the patch OK with on extra check
>
>   if (REG_P (xop0) || (SUBREG_P (xop0) && REG_P (SUBREG_REG (xop0))))

sorry, this extra check should be unnecessary. because the following
check will only
be true if the destination is register.

+  if (bitsize + bitnum > unit && bitnum < unit)
+    bitsize = unit - bitnum;

so I think the original patch is OK?

thanks.

Regards,
Jiong

>
> Regards,
> Jiong
>
>>
>> jeff
>>
>>
>>
>
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH/expand] PR64011 Adjust bitsize when partial overflow happen for big-endian
  2015-01-09 13:40   ` Jiong Wang
  2015-01-09 20:12     ` Jiong Wang
@ 2015-01-13 22:17     ` Jeff Law
  2015-01-13 22:47       ` Joseph Myers
  2015-01-14 23:15       ` Jiong Wang
  1 sibling, 2 replies; 13+ messages in thread
From: Jeff Law @ 2015-01-13 22:17 UTC (permalink / raw)
  To: Jiong Wang, gcc-patches

On 01/09/15 06:39, Jiong Wang wrote:

>
> the bug testcase is
> ===================
>
> typedef short U __attribute__((may_alias, aligned (1)));
> struct S
> {
>    _Complex float d __attribute__((aligned (8)));
> };
> void bar(struct S);
> void f5 (int x)
> {
>    struct S s;
>    ((U *)((char *) &s.d + 1))[3] = x;
>    bar (s);
> }

So I'm going to focus on that assignment statement.  Doesn't that write 
outside the bounds of "s"?    If I'm reading everything correctly we 
have an object that is 8 bytes (a complex float).  The assignment is a 2 
byte write starting at byte 7 in the object.  ISTM that writes one byte 
beyond the underlying object, at which point we're in undefined 
behaviour territory.

In many ways having the compiler or assembler spitting out an error here 
is preferable to silently compiling the code.  That would also help 
explain why we haven't seen this on other big endian targets with rich 
bitfield support (PA and H8 come to mind) -- it only arises in cases of 
undefined behaviour AFAICT.

What I do not like is the ICE or unrecognizable insn error.  The odds 
that someone running into those messages is going to realize it's 
because the input source is bogus is pretty small.

I'm really tempted here to use the conditional you want to add to 
store_bit_field_using_insv and when it triggers issue an error/warning 
instead of or in addition to truncating the size of the assignment.

Thoughts?

jeff



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH/expand] PR64011 Adjust bitsize when partial overflow happen for big-endian
  2015-01-13 22:17     ` Jeff Law
@ 2015-01-13 22:47       ` Joseph Myers
  2015-01-14  6:42         ` Jeff Law
  2015-01-14 23:15       ` Jiong Wang
  1 sibling, 1 reply; 13+ messages in thread
From: Joseph Myers @ 2015-01-13 22:47 UTC (permalink / raw)
  To: Jeff Law; +Cc: Jiong Wang, gcc-patches

On Tue, 13 Jan 2015, Jeff Law wrote:

> In many ways having the compiler or assembler spitting out an error here is
> preferable to silently compiling the code.  That would also help explain why

As usual, an error is incorrect in such a case that only has undefined 
behavior at runtime (but it may be compiled into an abort if the behavior 
is unconditionally undefined, and the abort doesn't replace anything 
before the undefined behavior that might have stopped the undefined 
behavior from occurring).

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH/expand] PR64011 Adjust bitsize when partial overflow happen for big-endian
  2015-01-13 22:47       ` Joseph Myers
@ 2015-01-14  6:42         ` Jeff Law
  0 siblings, 0 replies; 13+ messages in thread
From: Jeff Law @ 2015-01-14  6:42 UTC (permalink / raw)
  To: Joseph Myers; +Cc: Jiong Wang, gcc-patches

On 01/13/15 15:42, Joseph Myers wrote:
> On Tue, 13 Jan 2015, Jeff Law wrote:
>
>> In many ways having the compiler or assembler spitting out an error here is
>> preferable to silently compiling the code.  That would also help explain why
>
> As usual, an error is incorrect in such a case that only has undefined
> behavior at runtime (but it may be compiled into an abort if the behavior
> is unconditionally undefined, and the abort doesn't replace anything
> before the undefined behavior that might have stopped the undefined
> behavior from occurring).
You are, of course, correct.  We can't error here, but we can generate a 
conditional warning.

jeff

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH/expand] PR64011 Adjust bitsize when partial overflow happen for big-endian
  2015-01-13 22:17     ` Jeff Law
  2015-01-13 22:47       ` Joseph Myers
@ 2015-01-14 23:15       ` Jiong Wang
  2015-01-15  4:38         ` Jeff Law
  1 sibling, 1 reply; 13+ messages in thread
From: Jiong Wang @ 2015-01-14 23:15 UTC (permalink / raw)
  To: Jeff Law, gcc-patches


On 13/01/15 21:45, Jeff Law wrote:
> On 01/09/15 06:39, Jiong Wang wrote:
>
>> the bug testcase is
>> ===================
>>
>> typedef short U __attribute__((may_alias, aligned (1)));
>> struct S
>> {
>>     _Complex float d __attribute__((aligned (8)));
>> };
>> void bar(struct S);
>> void f5 (int x)
>> {
>>     struct S s;
>>     ((U *)((char *) &s.d + 1))[3] = x;
>>     bar (s);
>> }
> So I'm going to focus on that assignment statement.  Doesn't that write
> outside the bounds of "s"?    If I'm reading everything correctly we
> have an object that is 8 bytes (a complex float).  The assignment is a 2
> byte write starting at byte 7 in the object.  ISTM that writes one byte
> beyond the underlying object, at which point we're in undefined
> behaviour territory.
>
> In many ways having the compiler or assembler spitting out an error here
> is preferable to silently compiling the code.  That would also help
> explain why we haven't seen this on other big endian targets with rich
> bitfield support (PA and H8 come to mind) -- it only arises in cases of
> undefined behaviour AFAICT.
>
> What I do not like is the ICE or unrecognizable insn error.

currently, if a backend define "insv" with strict operand constraints to reject
negative imm, for example ARM, will ICE as unrecognizable error.

> I'm really tempted here to use the conditional you want to add to
> store_bit_field_using_insv and when it triggers issue an error/warning
> instead of or in addition to truncating the size of the assignment.

agree, and I think the truncation is needed otherwise there may have ICE on some target.

and I found current gcc LOCATION info is very good !
have done an experimental hack on at "expand_assignment": 4931 where the tree is expanded,
gcc could give quite useful & accurate warning based on tree LOCATION info.

./cc1 -O2 -mbig-endian pr48335-2.c

pr48335-2.c: In function ‘f5’:
pr48335-2.c:19:29: warning: overflow here !
    ((U *)((char *) &s.d + 1))[3] = x;
                              ^

while we need to add warning at store_bit_field_using_insv where
there is no accurate LOCATION info. but looks like it's acceptable?

pr48335-2.c:19:33: warning: overflow here !
    ((U *)((char *) &s.d + 1))[3] = x;
                                  ^

>
> Thoughts?
>
> jeff
>
>
>
>
>
>


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH/expand] PR64011 Adjust bitsize when partial overflow happen for big-endian
  2015-01-14 23:15       ` Jiong Wang
@ 2015-01-15  4:38         ` Jeff Law
  2015-01-15 16:36           ` Jiong Wang
  0 siblings, 1 reply; 13+ messages in thread
From: Jeff Law @ 2015-01-15  4:38 UTC (permalink / raw)
  To: Jiong Wang, gcc-patches

On 01/14/15 15:31, Jiong Wang wrote:
>
> agree, and I think the truncation is needed otherwise there may have ICE
> on some target.
>
> and I found current gcc LOCATION info is very good !
> have done an experimental hack on at "expand_assignment": 4931 where the
> tree is expanded,
> gcc could give quite useful & accurate warning based on tree LOCATION info.
>
> ./cc1 -O2 -mbig-endian pr48335-2.c
>
> pr48335-2.c: In function ‘f5’:
> pr48335-2.c:19:29: warning: overflow here !
>     ((U *)((char *) &s.d + 1))[3] = x;
>                               ^
>
> while we need to add warning at store_bit_field_using_insv where
> there is no accurate LOCATION info. but looks like it's acceptable?
>
> pr48335-2.c:19:33: warning: overflow here !
>     ((U *)((char *) &s.d + 1))[3] = x;
>                                   ^
Yes, I think we're on the right track now -- warn and truncate the the 
insertion.

I just scanned our set of warning flags to see if this would fit nicely 
under any of the existing flags, and it doesn't.  I guess putting it 
under -Wextra is probably best for now.

I think the warning text should indicate that the statement will write 
outside the bounds of the destination object or something along those lines.

Jeff

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH/expand] PR64011 Adjust bitsize when partial overflow happen for big-endian
  2015-01-15  4:38         ` Jeff Law
@ 2015-01-15 16:36           ` Jiong Wang
  2015-01-15 18:31             ` Jeff Law
  2015-01-15 22:16             ` Joseph Myers
  0 siblings, 2 replies; 13+ messages in thread
From: Jiong Wang @ 2015-01-15 16:36 UTC (permalink / raw)
  To: Jeff Law, gcc-patches

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


On 15/01/15 03:51, Jeff Law wrote:
> On 01/14/15 15:31, Jiong Wang wrote:
>> agree, and I think the truncation is needed otherwise there may have ICE
>> on some target.
>>
>> and I found current gcc LOCATION info is very good !
>> have done an experimental hack on at "expand_assignment": 4931 where the
>> tree is expanded,
>> gcc could give quite useful & accurate warning based on tree LOCATION info.
>>
>> ./cc1 -O2 -mbig-endian pr48335-2.c
>>
>> pr48335-2.c: In function ‘f5’:
>> pr48335-2.c:19:29: warning: overflow here !
>>      ((U *)((char *) &s.d + 1))[3] = x;
>>                                ^
>>
>> while we need to add warning at store_bit_field_using_insv where
>> there is no accurate LOCATION info. but looks like it's acceptable?
>>
>> pr48335-2.c:19:33: warning: overflow here !
>>      ((U *)((char *) &s.d + 1))[3] = x;
>>                                    ^
> Yes, I think we're on the right track now -- warn and truncate the the
> insertion.
>
> I just scanned our set of warning flags to see if this would fit nicely
> under any of the existing flags, and it doesn't.  I guess putting it
> under -Wextra is probably best for now.
>
> I think the warning text should indicate that the statement will write
> outside the bounds of the destination object or something along those lines.

thanks for suggestions. patch updated

the warning message is as the following:

./cc1 -O2 pr48335-2.c -Wextra

pr48335-2.c: In function ‘f5’:
pr48335-2.c:19:33: warning: write of 16bit data outside the bound of destination object, data truncated into 8bit [-Wextra]
    ((U *)((char *) &s.d + 1))[3] = x;
                                  ^
x86-64 bootstrap & regress ok.

ok for trunk?

one other thing is I found using of "insv" maybe sub-optimal in some situation.
for example, even before my patch, modify type of U to char so there is no overflow.

use BFI will cost one more fmov (cc1 -O2 test.c), maybe this is caused by bfi only
support integer type, which may cause extra reg copy when there are both float & int type.

// comment out the "if (maybe_expand_insn" in store_bit_field_using_insv
// to fall back to other code path
f5:
         uxtb    w0, w0
         stp     x29, x30, [sp, -16]!
         fmov    s0, wzr
         fmov    s1, w0
         add     x29, sp, 0
         bl      bar
         ldp     x29, x30, [sp], 16
         ret

// BFI version
f5:
         fmov    s0, wzr
         stp     x29, x30, [sp, -16]!
         add     x29, sp, 0
         fmov    w1, s0
         bfi     w1, w0, 0, 8
         fmov    s1, w1
         bl      bar
         ldp     x29, x30, [sp], 16
         ret


Thanks.

gcc/
     PR64011
     * expmed.c (store_bit_field_using_insv): Warn and truncate bitsize when there is
     partial overflow.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fix-overflow.patch --]
[-- Type: text/x-patch; name=fix-overflow.patch, Size: 1242 bytes --]

diff --git a/gcc/expmed.c b/gcc/expmed.c
index 0304e46..176c317 100644
--- a/gcc/expmed.c
+++ b/gcc/expmed.c
@@ -535,6 +535,21 @@ store_bit_field_using_insv (const extraction_insn *insv, rtx op0,
       copy_back = true;
     }
 
+  /* There are similar overflow check at the start of store_bit_field_1,
+     but that only check the situation where the field lies completely
+     outside the register, while there do have situation where the field
+     lies partialy in the register, we need to adjust bitsize for this
+     partial overflow situation.  Without this fix, pr48335-2.c on big-endian
+     will broken on those arch support bit insert instruction, like arm, aarch64
+     etc.  */
+  if (bitsize + bitnum > unit && bitnum < unit)
+    {
+      warning (OPT_Wextra, "write of "HOST_WIDE_INT_PRINT_UNSIGNED"bit data "
+	       "outside the bound of destination object, data truncated into "
+	       HOST_WIDE_INT_PRINT_UNSIGNED"bit", bitsize, unit - bitnum);
+      bitsize = unit - bitnum;
+    }
+
   /* If BITS_BIG_ENDIAN is zero on a BYTES_BIG_ENDIAN machine, we count
      "backwards" from the size of the unit we are inserting into.
      Otherwise, we count bits from the most significant on a

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH/expand] PR64011 Adjust bitsize when partial overflow happen for big-endian
  2015-01-15 16:36           ` Jiong Wang
@ 2015-01-15 18:31             ` Jeff Law
  2015-01-15 22:16             ` Joseph Myers
  1 sibling, 0 replies; 13+ messages in thread
From: Jeff Law @ 2015-01-15 18:31 UTC (permalink / raw)
  To: Jiong Wang, gcc-patches

On 01/15/15 09:27, Jiong Wang wrote:
>
> On 15/01/15 03:51, Jeff Law wrote:
>> On 01/14/15 15:31, Jiong Wang wrote:
>>> agree, and I think the truncation is needed otherwise there may have ICE
>>> on some target.
>>>
>>> and I found current gcc LOCATION info is very good !
>>> have done an experimental hack on at "expand_assignment": 4931 where the
>>> tree is expanded,
>>> gcc could give quite useful & accurate warning based on tree LOCATION
>>> info.
>>>
>>> ./cc1 -O2 -mbig-endian pr48335-2.c
>>>
>>> pr48335-2.c: In function ‘f5’:
>>> pr48335-2.c:19:29: warning: overflow here !
>>>      ((U *)((char *) &s.d + 1))[3] = x;
>>>                                ^
>>>
>>> while we need to add warning at store_bit_field_using_insv where
>>> there is no accurate LOCATION info. but looks like it's acceptable?
>>>
>>> pr48335-2.c:19:33: warning: overflow here !
>>>      ((U *)((char *) &s.d + 1))[3] = x;
>>>                                    ^
>> Yes, I think we're on the right track now -- warn and truncate the the
>> insertion.
>>
>> I just scanned our set of warning flags to see if this would fit nicely
>> under any of the existing flags, and it doesn't.  I guess putting it
>> under -Wextra is probably best for now.
>>
>> I think the warning text should indicate that the statement will write
>> outside the bounds of the destination object or something along those
>> lines.
>
> thanks for suggestions. patch updated
>
> the warning message is as the following:
>
> ./cc1 -O2 pr48335-2.c -Wextra
>
> pr48335-2.c: In function ‘f5’:
> pr48335-2.c:19:33: warning: write of 16bit data outside the bound of
> destination object, data truncated into 8bit [-Wextra]
>     ((U *)((char *) &s.d + 1))[3] = x;
>                                   ^
> x86-64 bootstrap & regress ok.
>
> ok for trunk?
>
> one other thing is I found using of "insv" maybe sub-optimal in some
> situation.
> for example, even before my patch, modify type of U to char so there is
> no overflow.
>
> use BFI will cost one more fmov (cc1 -O2 test.c), maybe this is caused
> by bfi only
> support integer type, which may cause extra reg copy when there are both
> float & int type.
>
> // comment out the "if (maybe_expand_insn" in store_bit_field_using_insv
> // to fall back to other code path
> f5:
>          uxtb    w0, w0
>          stp     x29, x30, [sp, -16]!
>          fmov    s0, wzr
>          fmov    s1, w0
>          add     x29, sp, 0
>          bl      bar
>          ldp     x29, x30, [sp], 16
>          ret
>
> // BFI version
> f5:
>          fmov    s0, wzr
>          stp     x29, x30, [sp, -16]!
>          add     x29, sp, 0
>          fmov    w1, s0
>          bfi     w1, w0, 0, 8
>          fmov    s1, w1
>          bl      bar
>          ldp     x29, x30, [sp], 16
>          ret
>
>
> Thanks.
>
> gcc/
>      PR64011
>      * expmed.c (store_bit_field_using_insv): Warn and truncate bitsize
> when there is partial overflow.
OK for the trunk.   Please install.

Thanks,
Jeff

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH/expand] PR64011 Adjust bitsize when partial overflow happen for big-endian
  2015-01-15 16:36           ` Jiong Wang
  2015-01-15 18:31             ` Jeff Law
@ 2015-01-15 22:16             ` Joseph Myers
  2015-01-16 10:45               ` [COMMITTED][PATCH/expand] " Jiong Wang
  1 sibling, 1 reply; 13+ messages in thread
From: Joseph Myers @ 2015-01-15 22:16 UTC (permalink / raw)
  To: Jiong Wang; +Cc: Jeff Law, gcc-patches

On Thu, 15 Jan 2015, Jiong Wang wrote:

> +  if (bitsize + bitnum > unit && bitnum < unit)
> +    {
> +      warning (OPT_Wextra, "write of "HOST_WIDE_INT_PRINT_UNSIGNED"bit data "
> +	       "outside the bound of destination object, data truncated into "
> +	       HOST_WIDE_INT_PRINT_UNSIGNED"bit", bitsize, unit - bitnum);

HOST_WIDE_INT_PRINT_UNSIGNED is a format for printf, not for the GCC 
diagnostic functions; in addition, the strings passed to the GCC 
diagnostic functions must be string constants, not concatenated with 
macros (only with other string constants directly appearing in the 
source), so that they can be extracted for translation.  You need to use 
%wu instead to print an unsigned HOST_WIDE_INT in a GCC diagnostic 
function such as warning.

(Also, there should be a hyphen between the number and "bit", "%wu-bit".)

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [COMMITTED][PATCH/expand] PR64011 Adjust bitsize when partial overflow happen for big-endian
  2015-01-15 22:16             ` Joseph Myers
@ 2015-01-16 10:45               ` Jiong Wang
  0 siblings, 0 replies; 13+ messages in thread
From: Jiong Wang @ 2015-01-16 10:45 UTC (permalink / raw)
  To: Joseph Myers; +Cc: Jeff Law, gcc-patches


On 15/01/15 21:56, Joseph Myers wrote:
> On Thu, 15 Jan 2015, Jiong Wang wrote:
>
>> +  if (bitsize + bitnum > unit && bitnum < unit)
>> +    {
>> +      warning (OPT_Wextra, "write of "HOST_WIDE_INT_PRINT_UNSIGNED"bit data "
>> +	       "outside the bound of destination object, data truncated into "
>> +	       HOST_WIDE_INT_PRINT_UNSIGNED"bit", bitsize, unit - bitnum);
> HOST_WIDE_INT_PRINT_UNSIGNED is a format for printf, not for the GCC
> diagnostic functions; in addition, the strings passed to the GCC
> diagnostic functions must be string constants, not concatenated with
> macros (only with other string constants directly appearing in the
> source), so that they can be extracted for translation.  You need to use
> %wu instead to print an unsigned HOST_WIDE_INT in a GCC diagnostic
> function such as warning.
>
> (Also, there should be a hyphen between the number and "bit", "%wu-bit".)

make sense, thanks, patch installed.

Index: gcc/expmed.c
===================================================================
--- gcc/expmed.c        (revision 219717)
+++ gcc/expmed.c        (working copy)
@@ -566,9 +566,9 @@
       etc.  */
    if (bitsize + bitnum > unit && bitnum < unit)
      {
-      warning (OPT_Wextra, "write of "HOST_WIDE_INT_PRINT_UNSIGNED"bit data "
-              "outside the bound of destination object, data truncated into "
-              HOST_WIDE_INT_PRINT_UNSIGNED"bit", bitsize, unit - bitnum);
+      warning (OPT_Wextra, "write of %wu-bit data outside the bound of "
+              "destination object, data truncated into %wu-bit",
+              bitsize, unit - bitnum);
        bitsize = unit - bitnum;
      }
  
Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog       (revision 219717)
+++ gcc/ChangeLog       (working copy)
@@ -1,5 +1,10 @@
  2015-01-15  Jiong Wang  <jiong.wang@arm.com>
  
+       * expmed.c (store_bit_field_using_insv): Improve warning message.
+       Use %wu instead of HOST_WIDE_INT_PRINT_UNSIGNED.
+
+2015-01-15  Jiong Wang  <jiong.wang@arm.com>
+
         PR rtl-optimization/64011
         * expmed.c (store_bit_field_using_insv): Warn and truncate bitsize when
         there is partial overflow.



^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2015-01-16 10:23 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-30 10:41 [PATCH/expand] PR64011 Adjust bitsize when partial overflow happen for big-endian Jiong Wang
2015-01-09  5:46 ` Jeff Law
2015-01-09 13:40   ` Jiong Wang
2015-01-09 20:12     ` Jiong Wang
2015-01-13 22:17     ` Jeff Law
2015-01-13 22:47       ` Joseph Myers
2015-01-14  6:42         ` Jeff Law
2015-01-14 23:15       ` Jiong Wang
2015-01-15  4:38         ` Jeff Law
2015-01-15 16:36           ` Jiong Wang
2015-01-15 18:31             ` Jeff Law
2015-01-15 22:16             ` Joseph Myers
2015-01-16 10:45               ` [COMMITTED][PATCH/expand] " Jiong Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).