Bug 88576 - -fno-math-errno causes GCC to consider that malloc does not set errno
Summary: -fno-math-errno causes GCC to consider that malloc does not set errno
Status: ASSIGNED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 9.0
: P3 normal
Target Milestone: ---
Assignee: Richard Biener
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2018-12-22 17:50 UTC by Aurelien Jarno
Modified: 2019-01-11 00:24 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2018-12-22 00:00:00


Attachments
patch (2.24 KB, patch)
2019-01-02 13:45 UTC, Richard Biener
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aurelien Jarno 2018-12-22 17:50:33 UTC
With the -fno-math-errno option, GCC optimizes-out saving and restoring errno around a malloc call. Here is a testcase, derived from the GNU libc string/strerror.c, to reproduce it:


typedef long unsigned int size_t;
  
extern char *__strerror_r (int __errnum, char *__buf, size_t __buflen);

extern void *malloc (size_t __size) __attribute__ ((__nothrow__)) __attribute__ ((__malloc__));

extern __thread int __libc_errno __attribute__ ((tls_model ("initial-exec")));

static char *buf;

char *strerror (int errnum)
{
  int saved_errno;
  saved_errno = __libc_errno;
  buf = malloc (1024);
  (__libc_errno = (saved_errno));
  return __strerror_r (errnum, buf, 1024);
}


Compile with: gcc -fmath-errno -Wall -O2 -fPIC -S -c strerror.i

Without -fno-math-errno, we can see in the output assembly code that errno is saved around the malloc call:

strerror:
.LFB0:
        .cfi_startproc
        pushq   %r12
        .cfi_def_cfa_offset 16
        .cfi_offset 12, -16
        pushq   %rbp
        .cfi_def_cfa_offset 24
        .cfi_offset 6, -24
        movl    %edi, %ebp
        movl    $1024, %edi
        pushq   %rbx
        .cfi_def_cfa_offset 32
        .cfi_offset 3, -32
        movq    __libc_errno@gottpoff(%rip), %rbx
        movl    %fs:(%rbx), %r12d
        call    malloc@PLT
        movl    %ebp, %edi
        movl    $1024, %edx
        movl    %r12d, %fs:(%rbx)
        movq    %rax, %rsi
        popq    %rbx
        .cfi_def_cfa_offset 24
        popq    %rbp
        .cfi_def_cfa_offset 16
        popq    %r12
        .cfi_def_cfa_offset 8
        jmp     __strerror_r@PLT
        .cfi_endproc


With -fno-math-errno, saving and restoring errno is optimized out:

strerror:
.LFB0:
        .cfi_startproc
        pushq   %rbx
        .cfi_def_cfa_offset 16
        .cfi_offset 3, -16
        movl    %edi, %ebx
        movl    $1024, %edi
        call    malloc@PLT
        movl    %ebx, %edi
        movl    $1024, %edx
        popq    %rbx
        .cfi_def_cfa_offset 8
        movq    %rax, %rsi
        jmp     __strerror_r@PLT
        .cfi_endproc


This is reproducible with 6.5, 7.4, 8.2 and a snapshot of trunk from 2018-12-17.
Comment 1 Andrew Pinski 2018-12-22 17:56:33 UTC
IIRC malloc setting errno also non standard.
Comment 2 Aurelien Jarno 2018-12-22 18:01:54 UTC
(In reply to Andrew Pinski from comment #1)
> IIRC malloc setting errno also non standard.

According to POSIX:

    The malloc() function shall fail if:

    [ENOMEM]
        [CX] [Option Start] Insufficient storage space is available. [Option End]

In that case malloc returns a NULL pointer. However even after adding a check for the return value, GCC still optimizes-out saving and restoring errno.
Comment 3 Andrew Pinski 2018-12-22 18:47:36 UTC
(In reply to Aurelien Jarno from comment #2)
> (In reply to Andrew Pinski from comment #1)
> > IIRC malloc setting errno also non standard.
> 
> According to POSIX:
> 
>     The malloc() function shall fail if:
> 
>     [ENOMEM]
>         [CX] [Option Start] Insufficient storage space is available. [Option
> End]
> 
> In that case malloc returns a NULL pointer. However even after adding a
> check for the return value, GCC still optimizes-out saving and restoring
> errno.

POSIX says one thing but C99 says another thing.
Comment 4 Andrew Pinski 2018-12-22 18:48:24 UTC
PR 42944
Comment 5 Zack Weinberg 2018-12-22 19:34:18 UTC
The C standard doesn't say malloc _will_ set errno on failure, but it also doesn't say it _won't_, and all library functions are allowed to clobber the value of errno unless it is specifically documented that they won't (N1570 7.5 [Errors], para 3, last sentence).

In any case, an option named -fno-math-errno has no business affecting the treatment of functions that have nothing to do with mathematics.
Comment 6 Florian Weimer 2018-12-22 19:39:28 UTC
Has this got to do anything with errno?  It seems to me that with -fno-math-errno, GCC assumes that malloc does not set *any* TLS variable.  That doesn't look right to me.
Comment 7 Aurelien Jarno 2018-12-22 19:43:53 UTC
(In reply to Zack Weinberg from comment #5)
> The C standard doesn't say malloc _will_ set errno on failure, but it also

Well at least POSIX says:

Otherwise, it shall return a null pointer and set errno to indicate the error. 

> doesn't say it _won't_, and all library functions are allowed to clobber the
> value of errno unless it is specifically documented that they won't (N1570
> 7.5 [Errors], para 3, last sentence).

I fully agree with that.
Comment 8 Zack Weinberg 2018-12-22 20:17:16 UTC
(In reply to Aurelien Jarno from comment #7)
> (In reply to Zack Weinberg from comment #5)
> > The C standard doesn't say malloc _will_ set errno on failure, but it also
> 
> Well at least POSIX says:
> Otherwise, it shall return a null pointer and set errno to indicate the
> error. 

Yeah, I wasn't denying that, I was responding to Andrew taking the attitude that this was fine because ISO C proper _didn't_ say that.

I dug into the code a little and it looks like this was an intentional re-use of -fno-math-errno to also mean "and neither will malloc", in the patch for PR 
42944.  I think that's wrong, but perhaps Richard Biener would like to argue otherwise...
Comment 9 Zack Weinberg 2018-12-22 20:22:08 UTC
... whoops, hit send a little too early.  AFAICT, the relevant code is call_may_clobber_ref_p_1 in tree-ssa-alias.c; I would say that the uses of flag_errno_math under the cases BUILT_IN_MALLOC, ALIGNED_ALLOC, CALLOC, STRDUP, STRNDUP, POSIX_MEMALIGN, REALLOC in that function are all wrong, and GCC should unconditionally assume errno may be clobbered by those builtins.  If this behavior is felt to be valuable, it should get its own -f switch.

(The uses of flag_errno_math under BUILT_IN_GAMMA*, LGAMMA*, and REMQUO* are appropriate, though, as those _are_ math functions.)
Comment 10 Andrew Pinski 2018-12-22 22:38:06 UTC
Read the thread starting at:
https://gcc.gnu.org/ml/gcc-patches/2018-10/msg00232.html
Comment 11 Richard Biener 2019-01-02 09:26:15 UTC
(In reply to Florian Weimer from comment #6)
> Has this got to do anything with errno?  It seems to me that with
> -fno-math-errno, GCC assumes that malloc does not set *any* TLS variable. 
> That doesn't look right to me.

GCC assumes that memory allocation does not affect global heap state.
That's correct unless you compile the memory allocator itself.

Note the original "alias" with -fno-math-errno was probably too lazy but
allocations being a barrier for any load/store of global memory is quite
bad for optimization in _fortran_ (which has no business with errno)
where we (unfortunately) also get everything passed by reference.

I'll introduce -f[no-]alloc-errno, defaulted to the -f[no-]math-errno
state and enabled at -Ofast (but not with -ffast-math).
Comment 12 Richard Biener 2019-01-02 13:45:52 UTC
Created attachment 45315 [details]
patch

What I have sofar - this doesn't work for Fortran behaving differently by default.  I think a proper fix would be to refactor the existing
_set_by_frontend stuff triggered via SetByCombined into a frontend specific
default_options_table (lang_hooks.default_options_table I guess) which
gets merged with the global default_options_table, overriding any settings
there.

Note there's a related bug complaining about -fno-math-errno not doing what
is documented.  That probably changed because of fortran as well.
Comment 13 Eric Gallager 2019-01-09 17:58:36 UTC
(In reply to Richard Biener from comment #11)
> (In reply to Florian Weimer from comment #6)
> > Has this got to do anything with errno?  It seems to me that with
> > -fno-math-errno, GCC assumes that malloc does not set *any* TLS variable. 
> > That doesn't look right to me.
> 
> GCC assumes that memory allocation does not affect global heap state.
> That's correct unless you compile the memory allocator itself.
> 
> Note the original "alias" with -fno-math-errno was probably too lazy but
> allocations being a barrier for any load/store of global memory is quite
> bad for optimization in _fortran_ (which has no business with errno)
> where we (unfortunately) also get everything passed by reference.
> 
> I'll introduce -f[no-]alloc-errno, defaulted to the -f[no-]math-errno
> state and enabled at -Ofast (but not with -ffast-math).

Darwin has -fno-math-errno set by default, but its malloc can set errno (to ENOMEM), so it wouldn't make sense to have -fno-alloc-errno set by -fno-math-errno on Darwin
Comment 14 Zack Weinberg 2019-01-11 00:24:49 UTC
I don't see why it would _ever_ make sense for -fno-alloc-errno to default to the setting of -fno-math-errno.  The math functions and the memory allocation functions are independent components of the C library.  Each toggle's default should be settable independently by the target configuration.