C++ `if` seems to be taking the wrong branch?

Question

I'm struggling with a non-sensical if statement...

Consider this code in a C++ file

if (coreAudioDevice) {
    delete coreAudioDevice;
    coreAudioDevice = nullptr;
}
coreAudioDevice = AudioDevice::GetDevice(defaultOutputDeviceID, false, coreAudioDevice, true);
if (coreAudioDevice)
{
    coreAudioDevice->setDefaultDevice(true);
    // we use the quick mode which skips initialisation; cache the device name (in AudioDevice)
    // using an immediate, blocking look-up.
    char devName[256];
    coreAudioDevice->GetName(devName, 256);

    AUDINFO ("Using default output device %p #%d=\"%s\".\n",
             defaultOutputDeviceID, coreAudioDevice, coreAudioDevice->GetName());
}
else
    AUDERR ("Failed to obtain a handle on the default device (%p)\n", coreAudioDevice);

calling a function in an ObjC++ file:

AudioDevice *AudioDevice::GetDevice(AudioObjectID devId, bool forInput, AudioDevice *dev, bool quick)
{
    if (dev) {
        if (dev->ID() != devId) {
            delete dev;
        } else {
            return nullptr;
        }
    }
    dev = new AudioDevice(devId, quick, forInput);
    return dev;
}

Which leads to the following terminal output:

ERROR coreaudio.cc:232 [init]: Failed to obtain a handle on the default device (0x7f81a1f1f1b0)

Evidently the if shouldn't fail because coreAudioDevice supposedly is NULL and then print a non-null value for this variable in the else branch.

I tried different compiler options and a different compiler (clang 4.0.1 vs. 5.0.1), apparently there is really something fishy in my code. Any thoughts?

What do you see when you put a breakpoint on the `return nullptr` line? That information should be pretty helpful — UnholySheep, Jan 16 '18 at 18:07
Consider putting brackets around the `else` branch. `AUDERR` is probably a macro and might be expanding to multiple statements. — Silvio Mayolo, Jan 16 '18 at 18:09
Why are you using a raw pointer at all? Looks like you want to have a `std::unique_ptr` instead. Also you should inspect your code using the debugger and step through line by line while obvserving how the variables are changing. — , Jan 16 '18 at 18:09
Just speculating based on what I think your code is trying to do: The `if (dev->ID() != devId) {` block looks suspicious to me. If the `AudioDevice` pointed to by `dev` already has ID that you want, shouldn't you return `dev` instead of `nullptr`? — 0x5453, Jan 16 '18 at 18:10
Indeed, thanks. Irrelevant here, but that was a regression introduced after trying different return types from this function. — RJVB, Jan 16 '18 at 21:09
> Why are you using a raw pointer at all? Because it may need to change. — RJVB, Jan 16 '18 at 21:12
Also, the debugger doesn't tell me anything useful that a print doesn't already tell me (i.e. the pointer isn't NULL). However, when I skip the test things go kaboom as soon as I try to call a method in the AudioDevice class. IOW, the pointer is invalid ... but `if` shouldn't know this! — RJVB, Jan 16 '18 at 21:26

score 1 · Answer 1 · answered Jan 16 '18 at 21:54

Reaching the end of the function without returning a value is undefined behavior in C++.

See http://en.cppreference.com/w/cpp/language/ub and What are all the common undefined behaviours that a C++ programmer should know about?.

So the call setDefaultDevice() can legally result in anything. The compiler is free to compile the program into an executable that can do anything, when the program's control flow leads to undefined behavior (i.e. the call to setDefaultDevice()).

In this case, entering the if block with coreAudioDevice non-zero leads to UB. So the optimizing compiler foresees this and chooses to then make it go into the else branch instead. Like this it can remove the first branch and the if entirely, to produce more optimized code.

See https://blogs.msdn.microsoft.com/oldnewthing/20140627-00/?p=633

Without optimizations the program should normally run as expected.

Slight nitpick: This is true only for functions with a non-`void` return type, except `main` (which implicitly returns 0). Also, not sure how you derived that the OP's first snippet is the very last thing in his function? — Tanz87, Jan 18 '18 at 22:56

score 0 · Answer 2 · answered Jan 16 '18 at 21:32

Well, at least I found a reason, but no understanding (yet).

I had defined this method, without noticing the compiler warning (amidst a bunch of deprecation warnings printed multiple times because of concurrent compilation...):

bool setDefaultDevice(bool isDefault)
{
    mDefaultDevice = isDefault;
}

Indeed, no return value.

Notice that I call this method inside the skipped if block - so theoretically I never got the chance to do that. BTW, it's what led me to discover this strange issue.

The issue goes away when I remove the call or when I make the method void as intended.

I think this also explains the very strange way of crashing I've seen: somehow the optimiser gets completely confused because of this. I'm tempted to call this a compiler bug; I don't use the return value from the method, so flow shouldn't be affected at all IMHO.

score 0 · Answer 3 · answered Jan 17 '18 at 10:19

Ah, right. Should I read that as "free to build an exec that can do anything EXCEPT the sensical thing"? If so, that former boss of mine had a point banning C++ as an anomaly (the exact word was French, "saleté")...

Anyway, I can understand why the behaviour would be undefined when you don't know a function doesn't actually put a return value on the stack. You'd be popping bytes off the stack after the return, messing things up. (Read heap for stack if necessary =] ). I'm guessing that's what would happen when you run this kind of code without optimisation, in good ole C or with the bugggy method defined out of line (so the optimiser cannot know that it's buggy).

But once you know that a function doesn't actually return a value and you see that the value wouldn't be used anyway, you should be able to emit code that doesn't pop the corresponding number of bytes. IOW, code that behaves as expected. With a big fat compile-time warning. Presuming the standard allows this that'd be the sensical thing to do, rather than optimise the entire tainted block away because that'd be faster. If that's indeed the reasoning followed in clang it doesn't actually inspire confidence...

Does the standard say this cannot be an error by default? I'd certainly prefer that over the current behaviour!

It only does this when optimizations are enabled (`-O#`), and with `-Wall` it warns about the UB. `-fsanitize=undefined` adds runtime checks for UB in clang. It is technically an error because it is not a valid program according to the standard. — tmlen, Jan 17 '18 at 11:01
Of course it's technically an error, IMHO for better reasons than UB due to assumptions on evaluation order. It's an error that's likely enough to happen that it should be raised as that. What's the difference with a non-optional function argument, after all? — RJVB, Jan 17 '18 at 15:50

C++ `if` seems to be taking the wrong branch?

3 Answers3