6

I am working on a rewrite of a raytracer that I developed for university last semester and I am running into the following problem: When I compile and run my code in Debug the output is as expected

expected result

But when I enable higher optimization levels e.g. "-O2" something completely different is the result:

actual result

And I am not sure why this happens. I tracked it down to the sphere intersection code

//#pragma GCC push_options
//#pragma GCC optimize("O0")

Intersection Sphere::intersect(const Ray& ray, const float previous) const
{
    const auto oc = ray.origin - center_;
    const auto lhv = -dot(ray.direction, oc);
    const auto discriminant = lhv * lhv - (oc.lensqr() - radius_ * radius_);

    if (discriminant < 0.0F)
    {
        return Intersection::failure();
    }
    float distance;
    const auto rhv = std::sqrt(discriminant);
    const auto r = std::minmax(lhv + rhv, lhv - rhv);
    if (r.first <= 0.0F)
    {
        if (r.second <= 0.0F)
        {
            return Intersection::failure();
        }
        distance = r.second;
    }
    else
    {
        distance = r.first;
    }

    const auto hit = ray.getPoint(distance);
    const auto normal = (hit - center_).normalize();

    if (0.0F <= distance && distance < previous - epsilon)
    {
        return {distance, ray, this, normal, hit};
    }
    return Intersection::failure();
}

//#pragma GCC pop_options

If I uncomment the pragma in release mode, I get the expected result again. Maybe I have some undefined behaviour in my code that leads to this?

You can also have a look here as a minimal reproducible example is not easily possible. https://github.com/Yamahari/RayTracer/blob/master/rt/solid/Sphere.cpp

(You can also clone the repo and build the project with cmake, you only need SFML as a dependency.

Use -DSFML_INCLUDE_DIR="include_dir" and -DSFML_LIB_DIR="lib_dir" with the sfml library compiled with your desired compiler)

Ðаn
  • 10,934
  • 11
  • 59
  • 95
Yamahari
  • 1,926
  • 9
  • 25
  • 1
    _Maybe I have some undefined behaviour in my code that leads to this?_ 99.44% likely. Have you tried turning on all compiler warnings? – Eljay Mar 25 '20 at 12:56
  • Yes, I am using '-Wall -Wextra -Wpedantic', and the only warnings I get is unused parameters and a non-standard gcc extension, but the code that uses that extension is not used in the example – Yamahari Mar 25 '20 at 12:59
  • 1
    I would use `-O3` and then disable all the disable-able warnings `-fno-blahblah`, then turn them on in batches until you can isolate the particular optimization(s). Once you have the optimization culprit(s), you may gain insight as to what undefined behavior the code is tripping over. Although, not all optimizations have an explicit compiler flag to enable/disable. The provide code snippet is not [mcve]. – Eljay Mar 25 '20 at 13:03
  • Can you step through the suspicious code with a debugger? Once in debug and once in release – Tom Mar 25 '20 at 13:15
  • Does `Intersection` have any reference members, or does the constructor make copies of all the parameters it gets? – 1201ProgramAlarm Mar 25 '20 at 13:17
  • Are you using `-ffast-math` in your optimized builds? That breaks strict conformance if IEEE floating points. https://stackoverflow.com/questions/7420665/what-does-gccs-ffast-math-actually-do. – NathanOliver Mar 25 '20 at 13:19
  • @1201ProgramAlarm copies all of them – Yamahari Mar 25 '20 at 13:33
  • @NathanOliver no, I am only passing '-O2', I also tried with '-fno-fast-math', did not help – Yamahari Mar 25 '20 at 13:33

1 Answers1

4

std::minmax returns a pair of references to its arguments. If you call it with prvalues as arguments, the references in this pair will be dangling after the end of the full-expression, which means that the access r.first in

if (r.first <= 0.0F)

will have undefined behavior here.

So store lhv + rhv and lhv - rhv in variables and call std::minmax on them or use

std::minmax({lhv + rhv, lhv - rhv})

to select the std::initializer_list overload, which returns a pair of actual values.


As noted by @Jarod42 in the comments, you don't actually need std::minmax here. rhv is the result of a std::sqrt call, so it is always non-negative, making lhv + rhv always the bigger (or equal) value.

walnut
  • 21,629
  • 4
  • 23
  • 59
  • 1
    @Yamahari Yes, it is not a good design choice. I think `std::minmax` should have another overload that returns by-value if either argument is an rvalue or alternatively the first overload should be SFINAEd if an argument is a rvalue. You might be better of writing such a version yourself, so that this cannot happen again. – walnut Mar 25 '20 at 14:21
  • Would `const auto [min, max] = std::minmax(lhv + rhv, lhv - rhv);` fix this? Aren't `min` and `max` immediately unpacking the returned values (which should still be valid, since the execution didn't reach the end of the full-expression which created the temporaries)? Given the fact that `auto` would **not** preserve references, I think it would work by copying the values. – Fureeish Mar 25 '20 at 14:28
  • 1
    @Fureeish No, structured bindings only refer into the members of the object they are bound to. No new objects are created for the individual members. So `const auto [min, max]` will materialize a `const std::pair` which lives until the scope ends, and `min` and `max` will refer to this `pairs`'s `first` and `second` members, which are (thereafter dangling) references. – walnut Mar 25 '20 at 14:35
  • Dang it, unfortunate. Thank you, though! – Fureeish Mar 25 '20 at 14:43
  • Yeah this is one of those really unfortunate places where you even use `auto` and you think you're fine and... you're not. – Barry Mar 25 '20 at 14:49
  • 2
    As `rhv` is result of `sqrt` so "positive", you might directly do `const auto [min, max] = std::pair(lhv - rhv, lhv + rhv);` – Jarod42 Mar 25 '20 at 14:51
  • @Jarod42 I didn't even notice that the `std::minmax` call wasn't needed. I have added that to my answer. – walnut Mar 25 '20 at 14:55