15

I found this problem in a very large application, have made an SSCCE from it. I don't know whether the code has undefined behavior or -O2 breaks it.

When compiling it with gcc a.c -o a.exe -O2 -Wall -Wextra -Werror it prints 5.

But it prints 25 when compiling without -O2 (eg -O1) or uncommenting one of the 2 commented lines (prevent inlining).

#include <stdio.h>
#include <stdlib.h>
// __attribute__((noinline)) 
int f(int* todos, int input) {
    int* cur = todos-1; // fixes the ++ at the beginning of the loop
    int result = input;
    while(1) {
        cur++;
        int ch = *cur;
        // printf("(%i)\n", ch);
        switch(ch) {
            case 0:;
                goto end;
            case 1:;
                result = result*result;
            break;
        }
    }
    end:
    return result;
}
int main() {
    int todos[] = { 1, 0}; // 1:square, 0:end
    int input = 5;
    int result = f(todos, input);
    printf("=%i\n", result);
    printf("end\n");
    return 0;
}

Is GCC's option -O2 breaking this small program or do I have undefined behavior somewhere?

5gon12eder
  • 24,280
  • 5
  • 45
  • 92
Bernd Elkemann
  • 23,242
  • 4
  • 37
  • 66
  • 5
    UB or not, I would recommend getting rid of `todos - 1` and using `int ch = *cur++;` instead of incrementing before dereference. – asveikau May 15 '14 at 15:52
  • 2
    According to GDB, when compiled with -O2, todos has the value {-5632, 0} ... but I can't understand why! – Kevin May 15 '14 at 15:55
  • 2
    Valgrind warns about `ch` being uninitialized.. can't understand why – Mark Nunberg May 15 '14 at 16:02
  • the output is not 'just' =5, it's =$input ! – Kevin May 15 '14 at 16:02
  • 3
    @ChrisStratton You seem to be on to something here. When I make `todo` as `{ -1, 1, 0 }` and then pass `&todos[1]` to `f()` the program yields the desired result – Mark Nunberg May 15 '14 at 16:04
  • 1
    I don't have gcc but the code looks fine to me. Have a look at the assembly output, try to put some printfs and try GDB although latter might be difficult with optimisations. – Jabberwocky May 15 '14 at 16:05
  • 1
    To be honest I'm not sure I should have voted to close it as a duplicate even if it may technically qualify... I think the fact there there is a formal issue is evidenced at the other question, but none of the answers are wonderfully satisfying. – Chris Stratton May 15 '14 at 16:05
  • 2
    so it is because of the `todos-1` even though it does not derefenrence? **interesting!** – Bernd Elkemann May 15 '14 at 16:06
  • @MichaelWalz yes. accepted ouah's answer which was quoting the *reason*. – Bernd Elkemann May 15 '14 at 16:13
  • Which version of gcc do you use on what platform ? – Jabberwocky May 15 '14 at 16:15
  • 1
    If you find the ANSI C Standard, this is explained with more verbosity, but, yes, you are invoking UB. From the ISO Standard appendix on UB: "Addition or subtraction of a pointer into, or just beyond, an array object and an integer type produces a result that does not point into, or just beyond, the same array object (6.5.6)." Keep in mind (N.B.) that 6.5.6 redefines non-array objects as single-element arrays. Truly, the ISO standard is terrible compared to its ANSI predecessor. Anyway, it is only recently that versions of GCC have become so draconian about UB as to break so much C code. – Heath Hunnicutt May 15 '14 at 16:19
  • @HeathHunnicutt +1 on that. A more common one is strict aliasing. – Mark Nunberg May 15 '14 at 17:46

2 Answers2

15
int* cur = todos-1;

invokes undefined behavior. todos - 1 is an invalid pointer address.

Emphasis mine:

(C99, 6.5.6p8) "If both the pointer operand and the result point to elements of the same array object, or one past the last element of the array object, the evaluation shall not produce an overflow; otherwise, the behavior is undefined."

ouah
  • 142,963
  • 15
  • 272
  • 331
  • 1
    even though it gets incremented again before the derefenrence? **interesting!** – Bernd Elkemann May 15 '14 at 16:10
  • 3
    @eznme you don't have to dereference it to invoke undefined behavior, C says the addition of the pointer and the integer here invokes the undefined behavior. – ouah May 15 '14 at 16:12
  • Yes, it's clearly UB. On the other hand, it is also stupid behaviour of GCC to optimize away part of that `todos[]` array because of that before-the-beginning pointer ref, **without warning the user**. – Kai Petzke Aug 22 '21 at 15:21
4

In supplement to @ouah's answer, this explains what the compiler is doing.

Generated assembler for reference:

  400450:       48 83 ec 18             sub    $0x18,%rsp
  400454:       be 05 00 00 00          mov    $0x5,%esi
  400459:       48 8d 44 24 fc          lea    -0x4(%rsp),%rax
  40045e:       c7 44 24 04 00 00 00    movl   $0x0,0x4(%rsp)
  400465:       00 
  400466:       48 83 c0 04             add    $0x4,%rax
  40046a:       8b 10                   mov    (%rax),%edx

However if I add a printf in main():

  400450:       48 83 ec 18             sub    $0x18,%rsp
  400454:       bf 84 06 40 00          mov    $0x400684,%edi
  400459:       31 c0                   xor    %eax,%eax
  40045b:       48 89 e6                mov    %rsp,%rsi
  40045e:       c7 04 24 01 00 00 00    movl   $0x1,(%rsp)
  400465:       c7 44 24 04 00 00 00    movl   $0x0,0x4(%rsp)
  40046c:       00 
  40046d:       e8 ae ff ff ff          callq  400420 <printf@plt>
  400472:       48 8d 44 24 fc          lea    -0x4(%rsp),%rax
  400477:       be 05 00 00 00          mov    $0x5,%esi
  40047c:       48 83 c0 04             add    $0x4,%rax
  400480:       8b 10                   mov    (%rax),%edx

Specifically (in the printf version), these two instructions populate the todo array

  40045e:       c7 04 24 01 00 00 00    movl   $0x1,(%rsp)
  400465:       c7 44 24 04 00 00 00    movl   $0x0,0x4(%rsp)

This is conspicuously missing from the non-printf version, which for some reason only assigns the second element:

  40045e:       c7 44 24 04 00 00 00    movl   $0x0,0x4(%rsp)
Mark Nunberg
  • 3,551
  • 15
  • 18
  • 2
    -1. This does not answer the question in a meaningful way. Except, of course, for people who can read assembly without problems. – Daniel Kamil Kozar May 15 '14 at 16:23
  • 1
    @DanielKamilKozar it is in addition ouah's answer, so it is fine to not repeat that only add what that answer didn't say. +1 – Bernd Elkemann May 15 '14 at 16:49
  • 2
    It sounds like what this is demonstrating, is that the issue isn't really that the pointer is being functionally broken at runtime, but rather that **the optimizing compiler is deciding that the first array element is never used (in a way that complies with the rules) and thus need not ever be initialized.** Declaring the array volatile might be another experiment to try. – Chris Stratton May 15 '14 at 16:52
  • @ChrisStratton That is very intersting. Since I dont want to add an answer to my own question maybe you would like to investigate that and write an answer? – Bernd Elkemann May 15 '14 at 17:04
  • I have no idea what this asm means – paulm May 15 '14 at 17:47
  • 1
    `%rsp` is the stack pointer. The `todo` array resides at `(%rsp)` or `0x0(%rsp)` or the beginning of the stack. `todo[1]` is four bytes into the stack, or `0x4(%rsp)`. The first example shows assignment of `$0x0` (`1`) to only `0x4(rsp)` => `todo[1] = 0`, while the second example assignments to both `$0x1` to `(%rsp)` (`todo[0]=1`) _and_ `0x0` to `0x4(%rsp)` (`todo[1]=0`) – Mark Nunberg May 15 '14 at 17:54