| Message ID | 54B7EA59.3060504@arm.com |
|---|---|
| State | New |
| Headers | show |
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
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".)
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