reinterpret_cast bug or UB?

Question

Consider following code:

#include <cstdint>
#include <algorithm>

std::uintptr_t minPointer(void *first, void *second) {
    const auto pair = std::minmax(
        reinterpret_cast<std::uintptr_t>(first),
        reinterpret_cast<std::uintptr_t>(second)
    );
    return pair.first;
}

and the assembly generated by GCC8 with -O3 on https://godbolt.org/z/qWJuV_ for minPointer:

minPointer(void*, void*):
  mov rax, QWORD PTR [rsp-8]
  ret

which clearly does not do what is intended by the code creator. Is this code causing some UB or is it GCC(8) bug?

AFAIK, you simply cannot use `<` to compare pointers if they are not in the same array or object; anything else is UB. — underscore_d, Sep 17 '18 at 18:56
(see comments below, but still somewhat related) [less than comparison for void pointers](https://stackoverflow.com/questions/24807453/less-than-comparison-for-void-pointers) — underscore_d, Sep 17 '18 at 18:57
@underscore_d Let's not encourage the misconception that C++ and C follow the same rules. — Brian Bi, Sep 17 '18 at 19:00
@underscore_d I agree with justin. Yet what You dropped is still pretty interesting and I did not know about it — bartop, Sep 17 '18 at 19:05
related: [Observing weird behavior with 'auto' and std::minmax](https://stackoverflow.com/questions/36555544/observing-weird-behavior-with-auto-and-stdminmax) / [If std::max() returns by reference (as it must), might that lead to a dangling reference?](https://stackoverflow.com/questions/13721839/if-stdmax-returns-by-reference-as-it-must-might-that-lead-to-a-dangling-r) - & cppreference for `minmax` notes - a bit far down - "_For overloads (1,2), if one of the parameters is an rvalue, the reference returned becomes a dangling reference at the end of the full expression that contains the call_" — underscore_d, Sep 17 '18 at 19:11
another: [structured bindings with std::minmax and rvalues](https://stackoverflow.com/questions/51503114/structured-bindings-with-stdminmax-and-rvalues) (spoiler: you can't avoid this with structured bindings, because if you declare the type as a value, that applies to the invisible `pair`, not its members) — underscore_d, Sep 17 '18 at 19:30

Barry · Accepted Answer · 2018-09-17T19:10:54.903

22

This is UB, but not for the reason you might think.

The relevant signature of std::minmax() is:

template< class T > 
std::pair<const T&,const T&> minmax( const T& a, const T& b );

In this case, your pair is a pair of references to uintptr_t const. Where are the actual objects we're referencing? That's right, they were temporaries created on the last line that have already gone out of scope! We have dangling references.

If you wrote:

return std::minmax(
    reinterpret_cast<std::uintptr_t>(first),
    reinterpret_cast<std::uintptr_t>(second)
).first;

then we don't have any dangling references and you can see that gcc generates appropriate code:

minPointer(void*, void*):
  cmp rsi, rdi
  mov rax, rdi
  cmovbe rax, rsi
  ret

Alternatively, you could explicitly specify the type of pair as std::pair<std::uintptr_t, std::uintptr_t>. Or just sidestep the pair entirely and return std::min(...);.

As far as language specifics, you are allowed to convert a pointer to a large enough integral type due to [expr.reinterpret.cast]/4, and std::uintptr_t is guaranteed to be large enough. So once you fix the dangling reference issue, you're fine.

edited Sep 17 '18 at 19:10

answered Sep 17 '18 at 19:07

Barry

286,269
29
621
977

1

[Minimal fix](https://godbolt.org/z/Ztq7B3). We really need lifetime dependency markup in C++. – Yakk - Adam Nevraumont Sep 17 '18 at 19:09
@Yakk-AdamNevraumont A custom decay mechanism would also be interesting, so that we could somehow specify that `pair` is a `pair` instead of `pair` here? – Barry Sep 17 '18 at 19:11
This is one of these things that seems so obvious in retrospect but also so easy to forget just once, with fatal results. :/ – underscore_d Sep 17 '18 at 19:15
1

@Barry `std::pair< T const&, T const& > minmax( T const& [[lifetime]], T const& [[lifetime]] )` (attributes probably not appropriate here) which states that the valid lifetime of the return value depends on the lifetimes of both input arguments. A compiler can then either (a) lifetime extend temporaries bound to input arguments, or (b) reject use-of-dangling-reference as unsafe. Return type changing depends on calling context, and that seems harder than this kind of markup. – Yakk - Adam Nevraumont Sep 17 '18 at 19:33
I'd like to see compilers warn about this – M.M Sep 17 '18 at 23:57
@M.M People relying on compilers to occasionally warn about dangling references sounds bad. `std::minmax` should just be banned outright. – Passer By Sep 18 '18 at 00:08

score 13 · Answer 2 · answered Sep 17 '18 at 19:06

The reinterpret_cast is well defined. The problem is that the type of const auto pair is const std::pair<const std::uintptr_t&, const std::uintptr_t&> as that's what std::minmax returns, so you have dangling references.

You just need to get rid of the dangling references for it to work:

std::uintptr_t minPointer(void *first, void *second) {
    const std::pair<std::uintptr_t, std::uintptr_t> pair = std::minmax(
        reinterpret_cast<std::uintptr_t>(first),
        reinterpret_cast<std::uintptr_t>(second)
    );
    return pair.first;
}

Godbolt link

reinterpret_cast bug or UB?

2 Answers2

Linked