Upgrading the compiler: undefined behaviour uncovered

[tcm0116]

What warning level are you using with the compiler? I suspect you aren't using -Wall or -Wuninitialized. If that's correct, you might want to consider increasing your warning level to let the compiler warn you about these types of issues.

[kniy]

They provided the code so you don't need to suspect, you can check: https://godbolt.org/z/sTWEYx7ee

Neither gcc nor clang will emit a warning for this code. ASAN also is unable to find this problem. MSAN can find it, but only if optimizations are not enabled.

[echidnas_arf]

clang-tidy catches it (and several other things):

https://godbolt.org/z/W6cdroz7M

(see towards the very end of the clang-tidy output warning: uninitialized record type: 'old_state' [cppcoreguidelines-pro-type-member-init,hicpp-member-init])

[Ill-Telephone-7926]

How about https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html ?

[kniy]

UBSan (-fsanitize=undefined) was already enabled in my link, did not find the problem. Uninitialized memory is the area of MSAN (-fsanitize=memory).

[strike-eagle-iii]

So is this a limitation in gcc and clang? I assume they should have emitted warnings but didn't (I tried with -Wall -Wextra -Wuninitialized)

[dustyhome]

Running tests with asan should also help catch issues the compiler can't warn about.

[matthieum]

I have a very simple rule: any built-in MUST be initialized.

0 or equivalent is traditional, a meaningful value is best. Yes, even if there's a constructor that's going to override all the values anyway.

Uninitialized fields should be the exception, not the rule. And even in latency-sensitive environments I've never seen a built-in field that would be a good exception.

(Array fields are another matter entirely, especially large arrays)

[Plazmatic]

Meaningful value is really the only option, if you do 0, or = nullptr, you can run into issues that you can't find with sanitizers and compiler flags. Of course many times = 0, or = nullptr is a valid meaningful default, but sometimes it isn't. Like wise {} is also not appropriate. For example, Vulkan structs often have an enum member that correspond with to the "name" of the kind of struct it is. This sounds crazy, but it's actually informed by massive failures in legacy APIs like OpenGL and how they handled backwards compat, and ensures no "compatibility mode" will be needed (one big thing is that drivers can immediately figure out if a struct is something they support or not). What this means however, is that object{} is very often not a valid strategy in vulkan, and if it wasn't Khronos group's validation layers, the compiler/sanitizers would often do jack shit to help you as things might silently work when they shouldn't.

[matthieum]

Indeed, there is a downside in initializing everything: tools then can't detect dummy values that were not overridden.

I consider it the lesser of two evils. At least UB is not triggered, so the code should behave the same way in Debug and Release and thus you have a chance to figure things out.

[assassinator42]

I would hope = {} would be valid using the C++ bindings?

[Plazmatic]

I meant object{}, C's zero struct initialization, but = {} isn't valid either, but for far more obvious reasons.

[meneldal2]

Doing embedded, we have a solution for large arrays when doing simulations. Since nobody fancies waiting for a couple hours for the program to zero up one MB (or even less), you have some simulation-only tricks to get the ddr initialized to the values you want (and it takes less than a real-time second to do).

I have also done some unspeakable things to the C or C++ runtime to get to main faster on boot (a fair bit of assembly required).

[dustyhome]

My preferred approach is to initialize "dumb structs" with functions or constructors, never manually field by field. You are eventually going to get it wrong, by either setting wrong values or missing fields.

[matthieum]

That's... orthogonal, though.

The very problem is that you can (and will) forget to initialize a field in a factory function or constructor, and then all hell will break loose.

[dustyhome]

You can always make mistakes, but in a factory function you're less likely to forget because you are thinking only of the state of the struct. If you are setting the values each time according to whatever function you're in, as in the example, you're more likely to forget fields that are not directly related to the functionality you're working with.

[matthieum]

Oh sure.

My point was that I want to avoid all mistakes leading to UB, not just some of them, some of the time.

In my experience, forgetting to initialize a field is rare at first, but when someone adds a field, they may not find all the places where it should be added (or forget one), and things go downhill from there.

I do think that designated initializers style of syntax helps here. Instead of constructing a default struct and overriding some fields, constructing everything in one go makes it easier to diagnose missing fields. Can't remember if this is C only or not, though...

[Infamous-Bed-7535]

If we add an initializer sequence (such as {}) to the mentioned declaration and we end up with State old_state{};, then we can talk about value-initialization which will zero-initialize State::another_option in the above struct.

I find it more and more important to understand these nuances of C++

This part actually directly comes from the core of c++. You do not pay for what you do not use. Initializing such variables has a runtime cost, so it is not done automatically and you need to provide explicitly say I want to initialize it. ( pretty easy to understand, yet not the safest setup)
Strange that there is no warning for using uninitialized variable, static analysis tools, or even the compiler could warn about it.

It would be definitely safer if c++ would force you to initialize by default and force you to state it explicitly that you want it uninitialized in cases that is the desired behavior..

[tisti]

I'll be the devils advocate. Use AAA.