| Message ID | 5311b2b4-3822-1996-5f80-e4afa2ea51f7@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | errno can't alias locals (PR 92412) | expand |
On Mon, Nov 11, 2019 at 11:38 PM Martin Sebor <msebor@gmail.com> wrote: > > The conditional in default_ref_may_alias_errno has the function > return true even for local variables, implying that locals must > be assumed not to have been changed across calls to errno-setting > functions like malloc. This leads to both worse code and also > false negatives in the strlen pass' detection of buffer overflow > across such calls. > > The attached patch constrains the conditional to only consider > external declarations. > > Tested on x86_64-linux. OK. This means a tentative definition for 'errno' is non-conforming? (besides not working well in practice, of course) Thanks, Richard. > > Martin
On Nov 12 2019, Richard Biener wrote:
> This means a tentative definition for 'errno' is non-conforming?
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/errno.h.html
If a macro definition is suppressed in order to access an actual
object, or a program defines an identifier with the name errno, the
behavior is undefined.
Andreas.
On Tue, Nov 12, 2019 at 11:13 AM Andreas Schwab <schwab@suse.de> wrote: > > On Nov 12 2019, Richard Biener wrote: > > > This means a tentative definition for 'errno' is non-conforming? > > https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/errno.h.html > > If a macro definition is suppressed in order to access an actual > object, or a program defines an identifier with the name errno, the > behavior is undefined. To quote the relevant parts before this: The <errno.h> header shall provide a declaration or definition for errno. The symbol errno shall expand to a modifiable lvalue of type int. It is unspecified whether errno is a macro or an identifier declared with external linkage. The first sentence suggests errno.h can provide a tentative definition (since that's also a definition?). The third sentence gives choice only between a macro and a declaration with external linkage (that's clearly _not_ a definition). So ... Richard. > Andreas. > > -- > Andreas Schwab, SUSE Labs, schwab@suse.de > GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 > "And now for something completely different."
On 11/12/19 1:04 AM, Richard Biener wrote: > On Mon, Nov 11, 2019 at 11:38 PM Martin Sebor <msebor@gmail.com> wrote: >> >> The conditional in default_ref_may_alias_errno has the function >> return true even for local variables, implying that locals must >> be assumed not to have been changed across calls to errno-setting >> functions like malloc. This leads to both worse code and also >> false negatives in the strlen pass' detection of buffer overflow >> across such calls. >> >> The attached patch constrains the conditional to only consider >> external declarations. >> >> Tested on x86_64-linux. > > OK. > > This means a tentative definition for 'errno' is non-conforming? > (besides not working well in practice, of course) Errno is a reserved name (just like all external identifiers) so only an implementation can declare it at file scope. But if you are asking if the patch changes the assumption GCC makes about errno aliasing tentative definitions, I don't think it does. AFAICT, GCC has been making the assumption since 4.5. Here's a slightly different test case than the one in pr92412 that makes it clearer: int errno; void* f (void) { int e = errno; void *p = __builtin_malloc (1); if (e != errno) // eliminated __builtin_abort (); return p; } But to be clear, my goal with the change is to let GCC (strlen specifically) assume that errno doesn't alias local variables. Martin
PR tree-optimization/92412 - excessive errno aliasing assumption defeats optimization gcc/ChangeLog: PR tree-optimization/92412 * targhooks.c (default_ref_may_alias_errno): Errono can only alias extern variables. gcc/testsuite/ChangeLog: PR tree-optimization/92412 * gcc.dg/strlenopt-91.c: New test. Index: gcc/targhooks.c =================================================================== --- gcc/targhooks.c (revision 278066) +++ gcc/targhooks.c (working copy) @@ -1415,9 +1415,11 @@ default_ref_may_alias_errno (ao_ref *ref) if (TYPE_UNSIGNED (TREE_TYPE (base)) || TYPE_MODE (TREE_TYPE (base)) != TYPE_MODE (integer_type_node)) return false; - /* The default implementation assumes an errno location - declaration is never defined in the current compilation unit. */ + /* The default implementation assumes an errno location declaration + is never defined in the current compilation unit and may not be + aliased by a local variable. */ if (DECL_P (base) + && DECL_EXTERNAL (base) && !TREE_STATIC (base)) return true; else if (TREE_CODE (base) == MEM_REF Index: gcc/testsuite/gcc.dg/strlenopt-91.c =================================================================== --- gcc/testsuite/gcc.dg/strlenopt-91.c (nonexistent) +++ gcc/testsuite/gcc.dg/strlenopt-91.c (working copy) @@ -0,0 +1,124 @@ +/* PR tree-optimization/92412 - excessive errno aliasing assumption defeats + optimization + { dg-do compile } + { dg-options "-O1 -Wall -fdump-tree-optimized" } */ + +typedef __SIZE_TYPE__ size_t; + +extern void* alloca (size_t); +extern void* calloc (size_t, size_t); +extern void* malloc (size_t); + +extern const char exta[4]; +static char stata[] = "123"; + +void sink (const void*, ...); + +#define T(ptr, alloc) do { \ + const char *p = ptr; \ + if (p[0] != '1' || p[1] != '2' || p[2] != '3' || p[3] != '\0' \ + || __builtin_strlen (p) != 3) \ + return; \ + \ + void *q = alloc; \ + __builtin_strcpy (q, p); \ + \ + if (p[0] != '1' || p[1] != '2' || p[2] != '3' || p[3] != '\0' \ + || __builtin_strlen (p) != 3 \ + || __builtin_strlen (q) != 3) \ + __builtin_abort (); \ + \ + sink (p, q); \ + } while (0) + + +void alloca_test_local (unsigned n) +{ + char loca[] = "123"; + T (loca, alloca (n)); +} + +void alloca_test_extern_const (unsigned n) +{ + T (exta, alloca (n)); +} + +void alloca_test_static (unsigned n) +{ + T (stata, alloca (n)); +} + + +// Verify fix for PR tree-optimization/92412. +void calloc_test_local (unsigned m, unsigned n) +{ + char loca[] = "123"; + T (loca, calloc (m, n)); +} + +void calloc_test_extern_const (unsigned m, unsigned n) +{ + T (exta, calloc (m, n)); +} + +void calloc_test_static (unsigned m, unsigned n) +{ + T (stata, calloc (m, n)); +} + + +// Verify fix for PR tree-optimization/92412. +void malloc_test_local (unsigned n) +{ + char loca[] = "123"; + T (loca, malloc (n)); +} + +void malloc_test_extern_const (unsigned n) +{ + T (exta, malloc (n)); +} + +void malloc_test_static (unsigned n) +{ + T (stata, malloc (n)); +} + + +#undef T +#define T(ptr, n) do { \ + const char *p = ptr; \ + if (p[0] != '1' || p[1] != '2' || p[2] != '3' || p[3] != '\0' \ + || __builtin_strlen (p) != 3) \ + return; \ + \ + char vla[n]; \ + char *q = vla; \ + __builtin_strcpy (q, p); \ + \ + if (p[0] != '1' || p[1] != '2' || p[2] != '3' || p[3] != '\0' \ + || __builtin_strlen (p) != 3 \ + || __builtin_strlen (q) != 3) \ + __builtin_abort (); \ + \ + sink (p, vla); \ + } while (0) + + +void vla_test_local (unsigned n) +{ + char loca[] = "123"; + T (loca, n); +} + +void vla_test_extern_const (unsigned n) +{ + T (exta, n); +} + +void vla_test_static (unsigned n) +{ + T (stata, n); +} + +/* { dg-final { scan-tree-dump-not "abort" "optimized" } } */