Compile-time checks to ensure correct behavior for pointer arithmetic

[braydenpl]

Motivation

This pull-request aims to resolve an instance of undefined behavior in the definition of ImVec2 while maintaining compatibility with C++11.

The UB is in these lines:

float                                   x, y;

float& operator[] (size_t idx)          { IM_ASSERT(idx == 0 || idx == 1); return ((float*)(void*)(char*)this)[idx]; }
float  operator[] (size_t idx) const    { IM_ASSERT(idx == 0 || idx == 1); return ((const float*)(const void*)(const char*)this)[idx]; }

where the ImVec2 instance is treated as an array of floats through type-punning. Most compilers will lay out float x,y contiguously (which is why this hasn't cropped up as an issue yet), but padding between any and all struct fields is implementation-defined. It is thus UB to access x and y this way, which is an issue because any tweaks to compilers' optimization algorithms could result in all code using ImVec2 to stop working upon recompilation.

Proposed Solution

I have rewritten these subscript operations to preserve the API and be as efficient, if not faster, than the previous implementation. The IM_ASSERT is replaced by taking idx modulo 2, which provides the same guarantee as before, but also makes it possible for invalid subscripts to reduce to valid ones. This replaces a segfault with just quietly working but potentially giving unintended values.

Compilers and linters don't always realize that a size_t mod 2 will always be in {0,1}, so I chose to make the default case unreachable as a further hint for the optimizer and also for the reader. This required an unreachable function, which is provided by the stdlib in C++23 but only by compiler intrinsics for C++11. So, I added ImGui::Unreachable() to imgui.h to provide the tool for the rest of the library and community so that others can access and use it without having to redefine it everywhere (it's quite useful).

Experiments and Testing

Experimentation across C++ versions and optimization levels for alternative implementations was done with compiler explorer (work linked).

Additionally, I ran the imgui_test_suite on both the current master branch and my fork. This branch performed identically (passed the same tests) as master, so I believe merging would not cause any regressions.

Tests were run on hardware and inside valgrind to attain diagnostic information. Presently, both my branch and master fail the same two tests (capture_readme_gif and capture_faq_idstack_gif), but it seems from the dumps that the issue is an ffmpeg one and is unrelated to this work. I will dig further into it tomorrow to see what's wrong.

Edit: The ffmpeg errors were because I didn't run the test engine in a debugger. I've added a page to the test engine wiki for future reference https://github.com/ocornut/imgui_test_engine/wiki/Testing-Dear-ImGui.

[achabense]

Though access from reinterpreted this is truely UB... I think it's enough to keep the old assertion and just return idx == 0 ? x : y.

[ocornut]

Hello,

Thanks for your PR and generally being precise and detailed.

However, I am afraid this seems overkill, betrays some of Dear ImGui design (e.g. including <utility>) and almost certainly real-world unnecessary.

Out of curiosity, were you actually trying to solve a real issue you were having (e.g. some tool complaining)? Or is this purely theoretical? If the earlier I would definitively want to find a solution. If you start chasing theoretical problems you are in for a long and unproductive ride. Unusual alignment padding would almost certainly be an larger issue for a million other reasons, but the expression could be reworked to use offsetof(y)-offsetof(x) as well.

This replaces a segfault with just quietly working but potentially giving unintended values.

This is not a desirable error handling design for this project so we'd generally want to keep the assert.

Experimentation across C++ versions and optimization levels for alternative implementations was done with compiler explorer (work linked).

As stated by the comment this specific helper is not particularly critical, hence the presence of an assert. But in principle we are equally careful about non-optimized builds too. Original code naturally emit branch-less code.

I think it's enough to keep the old assertion and just return idx == 0 ? x : y.

This seems like a better choice indeed.

-Omar

[braydenpl]

Thank you for your reply! I do agree that the ternary-if is a nicer solution given the context. I wasn't sure about how truly non-critical the helper was, so I chose to err on the side of caution.

UB is UB, so this falls firmly into the real-world camp of issues. Wrt <utility>, it would only be included if a user enabled it. But of course this is just a clarifying remark since I accept achabense's solution as better.

Should I add further commits to negate these and implement the alternative instead? Or should I just close the PR and you will write in the change?

I look forward to further contribution on this project.

[ocornut]

UB is UB, so this falls firmly into the real-world camp of issues.

It's really not - until proven that this is affecting you or someone (without intently crafting a dedicated situation to prove the point) that's not my definition of real-world, but a possible definition of language lawyers having too much time on their hands. Heck, a majority of our multi-components function e.g. SliderFloat3() are expecting zero-padding between floats. I don't disagree that your solution is in theory more correct but it's not a good tradeoff for the codebase which wants other qualities (short code, ease of reading etc.).

Should I add further commits to negate these and implement the alternative instead? Or should I just close the PR and you will write in the change?

Well my issue is that the alternative doesn't provide a branch-free version, so even if the code is not performance critical, we'd be applying a change that has a known negative performance effect to solve an imaginary problem, which is odd. At the first glimpse that there is a detectable real-world problem I would likely adopt it, tho.

[braydenpl]

Edit: the results of these tests are useless, retesting is done here

Omar,

I appreciate your perspective. I'm not interested in imaginary problems either, so I profiled the possible changes to consider their real-world performance impacts.

Possible solutions

• Control: no changes
• Unreachable: the change involving a switch case
• Branching: replace the type-punning with a ternary-if
• static_assert: add static_assert(offsetof(ImVec2, y) - offsetof(ImVec2, x) == sizeof(float)); after the ImVec2 definition.

Experiment

• Performance and regression tests were performed using the test suite portion of the test engine. Each alternative solution was built with both default settings and with -O3. Debug results and release results were compared separately. To acquire full results (catching all breakpoints), all tests were conducted in gdb.

• Environment: GNU gdb (GDB; openSUSE Tumbleweed) 16.3 | OS: x86_64 Linux | CPU: AMD Ryzen 7 PRO 5850U | GPU: Radeon Vega 8 | RAM: 32 GB

Results

Although results of the unoptimized build are included for completeness, I am primarily considering optimized performances. I analyzed the data and produced the graphs in this spreadsheet.

It is unsurprising that static_assert performs identically to the control run. Most interesting is how branching outperforms everything else, though not exactly surprising because this kind of ternary-if expression is common enough an idiom to be a heuristic for optimization. I feel this is indeed is a valuable insight to the contrary of "a known negative performance effect," or at least that it's often better to measure than to have a philosophical argument. (edit)

Discussion

Returning to the original discussion, there are now two (or one-and-a-half) issues:

1. There is theoretically-incorrect code;
2. The incorrect code can be replaced with code that is more: correct, performant, legible, and concise.

I've looked further throughout the codebase to try and identify similar indexing practice but turned up short.

Heck, a majority of our multi-components function e.g. SliderFloat3() are expecting zero-padding between floats.

I investigated that family of functions (of which arrays float v[3] are a parameter) and am unsure of how they're problematic. Arrays are required to have contiguous elements. The issue is that structs don't have that guarantee so we have to be more careful (e.g. provide constraints like the static_assert). However, I could be missing something obvious.

In light of these results I will update my PR to use the ternary-if expressions as described by @achabense, which is indeed the best of the options available.(edit)

Edit: I have now committed the changes and reopened the PR for review.

Brayden

[achabense]

There used to be an issue of the same problem. #6272

[ocornut]

I appreciate your perspective. I'm not interested in imaginary problems either, so I profiled the possible changes to consider their real-world performance impacts.

It's unclear what you actually measured: all those perf_xxxx tests are general code exercising, where this operator[] is barely and certainly not meaningfully used. For a good portion of those tests the operator will be called close to zero times and not nearly to an amount that's even measurable, so most likely results are noise.

I'm mostly happy/excited here that you tried those tools in the test engine / test suite, you may be the first human other than me and Rokas to even look at them :)

this kind of ternary-if expression is common enough an idiom to be a heuristic for optimization. I feel this is indeed is a valuable insight to the contrary of "a known negative performance effect," or at least that it's often better to measure than to have a philosophical argument

To clarify, it's not expected that any of this would have a real-world performance impact given the current codebase. But if you look at assembly output one is a branchless address calculation and one is a branch which has a roughly 50% likelihood of misprediction. Out of principle I'm reluctant to make changes simply to cater to motivations which I find to be cargo-cult. I went through this in #6272.

Here's a genuine question: how did you end up finding this and wanting to make this change? Was it detected by a e.g. static analysis tool?

[braydenpl]

It's unclear what you actually measured: all those perf_xxxx tests are general code exercising, where this operator[] is barely and certainly not meaningfully used. For a good portion of those tests the operator will be called close to zero times and not nearly to an amount that's even measurable, so most likely results are noise.

Thank you, at this tip I redid the experiment and the previous results were totally useless. Here are the details on the new tests (done as microbenchmarks). I was quite mistaken about how an optimizer would deal with that branch (embarrassing, sorry for being confidently incorrect).

Static_Assert performs exactly the same as Control, so Control gets covered on the dot graph (you can kind of see the blue in parts).

I changed my PR to leave the implementation untouched and just put a compile-time check for the correct padding after the ImVec2 definition. In retrospect this is the most obvious and cleanest thing to do. Admittedly I'm getting back into writing C++ regularly after a while of not doing it, and I apologize for burdening the project with hair-brained first drafts.

Out of principle I'm reluctant to make changes simply to cater to motivations which I find to be cargo-cult. I went through this in #6272.

Sorry to make you rehash all of this, I didn't see that issue when searching.

Here's a genuine question: how did you end up finding this and wanting to make this change? Was it detected by a e.g. static analysis tool?

Clang-tidy alerted me to it. To minimally reproduce, you can run:

clang-tidy imgui.h -checks=cppcoreguidelines-pro-bounds-* -- -x c++ --std=c++11

Thank you for your time and continued patience,

Brayden

[ocornut]

Clang-tidy alerted me to it. To minimally reproduce, you can run:

I am away so partial answer but that’s valuable info. Can you share the output ?
Does the compile time check for padding has an effect on this Clang-tidy run?

About the eg SliderFloat3 etc function it is because we also promote passing eg &vec4.x as the start address for those.

[braydenpl]

Here's the relevant part of the output:

.../imgui.h:300:87: warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
  300 |     float& operator[] (size_t idx)          { IM_ASSERT(idx == 0 || idx == 1); return ((float*)(void*)(char*)this)[idx]; } // We very rarely use this [] operator, so the assert overhead is fine.
      |                                                                                       ^
.../imgui.h:301:87: warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
  301 |     float  operator[] (size_t idx) const    { IM_ASSERT(idx == 0 || idx == 1); return ((const float*)(const void*)(const char*)this)[idx]; }

There's quite a lot more output but this is right at the top and the struct accessing stood out to me. Invoking this tool gives a lot of false positives.

Does the compile time check for padding has an effect on this Clang-tidy run?

Unfortunately the check doesn't have an effect on clang-tidy.

I'm mostly happy/excited here that you tried those tools in the test engine / test suite, you may be the first human other than me and Rokas to even look at them :)

The tests are great! I'm shocked that so few are using them. Fwiw it feels really nice to have a drop-in automated test suite.

About the eg SliderFloat3 etc function it is because we also promote passing eg &vec4.x as the start address for those.

Thanks for the clarification. I'll add a similar check to ImVec4 and you can judge how the constraint model fits in.

Brayden