struct A { char a1[1]; char a2[5]; char a3[1]; char a4[2048 - 7]; } a; typedef __SIZE_TYPE__ size_t; extern void *memset (void *, int, size_t); extern void *memcpy (void *, const void *, size_t); extern int memcmp (const void *, const void *, size_t); extern void abort (void); void bar (struct A *x) { size_t i; if (memcmp (x, "\1HELLO\1", sizeof "\1HELLO\1")) abort (); for (i = 0; i < sizeof (x->a4); i++) if (x->a4[i]) abort (); } int foo (void) { memset (&a, 0, sizeof (a)); a.a1[0] = 1; memcpy (a.a2, "HELLO", sizeof "HELLO"); a.a3[0] = 1; bar (&a); return 0; } int main (void) { foo (); return 0; } is miscompiled on ppc-linux at -O2 and -O3 (assuming the testcase is valid). The 2 memcpy (a.a2, ...) instructions get swapped with a.a3[0] = 1 insn during sched2, because nonoverlapping_memrefs_p says: (mem/s:HI (plus:SI (reg/f:SI 29 29 [120]) (const_int 5 [0x5])) [0 a.a2+4 S2 A8]) and (mem/s:QI (plus:SI (reg/f:SI 29 29 [120]) (const_int 6 [0x6])) [0 a.a3+0 S1 A8]) don't overlap (as they have recorded different fields of the same structure). The patch that introduced this optimization was: http://gcc.gnu.org/ml/gcc-patches/2001-12/msg00072.html Now, is that valid C to overflow from one field into another one within the same structure? If yes, I think nonoverlapping_memrefs_p would need to take into account offsets, sizes and relative distance of the fields. If not, then perhaps glibc -D_FORTIFY_SOURCE=2 should use __builtin_offset_size (dst, 1) rather than (dst, 0) even for memcpy/etc.
memcpy (a.a2, "HELLO", sizeof "HELLO"); That is invalid as a.a2 is only 5 in size and "HELLO" is 6 in size.
> That is invalid as a.a2 is only 5 in size and "HELLO" is 6 in size. I know it overlaps into the next field. Whether it is invalid is unclear to me. How about memcpy ((char *) &a + offsetof (struct A, a2), "HELLO", 6)? Is there supposed to be any semantic difference between the 2?
Subject: Re: nonoverlapping_memrefs_p returns true even for overlapping memory references > > > ------- Additional Comments From jakub at gcc dot gnu dot org 2005-08-25 15:47 ------- > > That is invalid as a.a2 is only 5 in size and "HELLO" is 6 in size. > I know it overlaps into the next field. Whether it is invalid is unclear to me. > How about memcpy ((char *) &a + offsetof (struct A, a2), "HELLO", 6)? > Is there supposed to be any semantic difference between the 2? yes because one talks about the object a and the other one talks about the object a.a2. This is at least as I understand what the C standard says. -- Pinski
Careful, Andrew. Things are not as cut-and-dried as you're making it out. Indeed, this is yet another example of the big structure member aliasing discussion we had earlier this year. I can't find the reference just now. I vaguely seem to recall that for the nonce we decided to allow up-casts from members back to the structure. Which suggests that our string builtins should adjust the MEM_EXPR to discard the member from the aliasing info, and refer only to the object.
All that said, I personally would consider this a source code bug. If you really meant to initialize two members of the structure, I think it makes logical sense that you refer to the object as a whole. Otherwise we deny the ability to apply sensible range checks to array members within objects. And in the case in question, it's quite obviously an off-by-one bug on the part of the programmer. They did not really intend to initialize a3[0] twice. So I think it would be useful if _FORTIFY_SOURCE complained about this usage even if it turns out to be within the letter of the law.
Subject: Re: nonoverlapping_memrefs_p returns true even for overlapping memory references rth at gcc dot gnu dot org wrote: > And in the case in question, it's quite obviously an off-by-one bug on the > part of the programmer. They did not really intend to initialize a3[0] twice. > So I think it would be useful if _FORTIFY_SOURCE complained about this usage > even if it turns out to be within the letter of the law. We did seem to reach the consensus that it was OK to upcast from a member of the structure to the containing structure, or, rather, that there was nothing that definitively made that invalid. This is a bit different, in that the problematic memcpy is not mentioning a3 at all; it's just stepping on it. I'm not sure whether this case is valid; my guess is that it is, simply in that the C standard says so little about the object model that one rather has to assume such things are legal. Then again, you're not strictly pseaking allowed to index off the end of an array, so I'm not sure. However, if memcpy were an arbitrary function, then by the conclusion in the first paragraph, it certainly might modify "a.a3". So, the compiler must be making some special assumption about memcpy. I'd suggest ceasing to make that assumption, in the name of caution. I agree that in an error-checking capacity it makes sense to warn. As RTH says, this is not something that programmers mean to do.
Ok, I'll play with get_memory_rtx.
Subject: Bug 23561 CVSROOT: /cvs/gcc Module name: gcc Changes by: jakub@gcc.gnu.org 2005-08-26 22:02:45 Modified files: gcc : ChangeLog builtins.c gcc/testsuite : ChangeLog Added files: gcc/testsuite/gcc.c-torture/execute: 20050826-1.c Log message: PR rtl-optimization/23561 * builtins.c (get_memory_rtx): Add LEN argument. If MEM_EXPR is a COMPONENT_REF, remove all COMPONENT_REF from MEM_EXPR unless at most LEN bytes long memory fits into the field. (expand_builtin_memcpy, expand_builtin_mempcpy, expand_movstr, expand_builtin_strncpy, expand_builtin_memset, expand_builtin_memcmp, expand_builtin_strcmp, expand_builtin_strncmp): Adjust callers. * gcc.c-torture/execute/20050826-1.c: New test. Patches: http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.9833&r2=2.9834 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/builtins.c.diff?cvsroot=gcc&r1=1.472&r2=1.473 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&r1=1.5968&r2=1.5969 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.c-torture/execute/20050826-1.c.diff?cvsroot=gcc&r1=NONE&r2=1.1
Subject: Bug 23561 CVSROOT: /cvs/gcc Module name: gcc Branch: gcc-4_0-branch Changes by: jakub@gcc.gnu.org 2005-08-27 09:41:08 Modified files: gcc : ChangeLog builtins.c gcc/testsuite : ChangeLog Added files: gcc/testsuite/gcc.c-torture/execute: 20050826-1.c Log message: PR rtl-optimization/23561 * builtins.c (get_memory_rtx): Add LEN argument. If MEM_EXPR is a COMPONENT_REF, remove all COMPONENT_REF from MEM_EXPR unless at most LEN bytes long memory fits into the field. (expand_builtin_memcpy, expand_builtin_mempcpy, expand_movstr, expand_builtin_strncpy, expand_builtin_memset, expand_builtin_memcmp, expand_builtin_strcmp, expand_builtin_strncmp): Adjust callers. * gcc.c-torture/execute/20050826-1.c: New test. Patches: http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=2.7592.2.393&r2=2.7592.2.394 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/builtins.c.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=1.426.2.3&r2=1.426.2.4 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=1.5084.2.353&r2=1.5084.2.354 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.c-torture/execute/20050826-1.c.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=NONE&r2=1.1.2.1
Should be fixed on 4.0/HEAD.