facebook / yoga

Yoga is an embeddable layout engine targeting web standards.
https://yogalayout.dev/
MIT License
17.25k stars 1.42k forks source link

[c++] YGNodeCalculateLayout() is not working on iOS/iPad with any optimization -O1, O2, O3, OFast (because of finite-math-only) #1679

Closed Wiksnet closed 1 month ago

Wiksnet commented 3 months ago

Report

Hi Team,

Could you please take a look at the issue with NaN values? I believe that using std::optional or a smaller number instead of NaN for YGUndefined would resolve the problem. The current use of NaN leads to undefined behavior on different processors when fast-math is enabled.

Issues and Steps to Reproduce

  1. Create any simple layout with child Nodes.
  2. Execute YGNodeCalculateLayout(node, width, height, YGDirectionLTR);
  3. Do printf for child elements. YGNodeLayoutGetWidth(node) or YGNodeLayoutGetLeft(node)
  4. Compile with -O3 or -OFast
  5. Run on iPad M2 for example or A12X (actually it can be even iPhone)
  6. Result:
Root x: 0.000000 y: 0.000000, w: 1366.000000, h: 1024.000000
child x: nan, y: nan, w: nan, h: nan
child x: nan, y: nan, w: nan, h: nan
child x: 0.000000, y: 0.000000, w: nan, h: nan
child x: 0.000000, y: 0.000000, w: nan, h: nan
child x: nan, y: nan, w: nan, h: nan
child x: nan, y: nan, w: nan, h: nan
child x: nan, y: nan, w: nan, h: nan
child x: nan, y: nan, w: nan, h: nan

Expected Behavior

No NaN values for the YGNodeLayoutGetWidth or YGNodeLayoutGetHeight If the flag is enabled: -ffast-math

Actual Behavior

Without -fno-finite-math-only it can't work. For any Node I'm getting Nan value. Actually this is because you are using constexpr float YGUndefined = std::numeric_limits<float>::quiet_NaN(); which is not working with fast-math. To fix that temporary I have to use -fno-finite-math-only But this significantly affects the performance of my audio application where unroll loops disappear due to these checks with Nan.

Thank you! p.s. on M1 (arm) everything works fine.

NickGerleman commented 3 months ago

Changing YGUndefined to be non-NaN would be a major breaking change. There is a lot of code out there, even outside of Yoga, and its bindings, which rely on not just the type, but the value (e.g. mixing and matching yoga is undefined checks, with manual math against NaN).

-O1, -O2, -Os, and I think even -O3 does not enable -ffast-math. Just -Ofast, which is explicitly a bit dangerous to use since it causes non-standards behavior.

Would it be possible in your setup to avoid any fast math flags, just for Yoga? I don't think the work, and downstream pain of changing representation would be worth the benefit to the small number of users who use fast FP.

Wiksnet commented 3 months ago

@NickGerleman Thanks for quick answer. The issue is not only with -Ofast. Any type of optimization even lower like -01 or -Os makes Yoga broken.

Today I tested it on Mac OS X (M1 arm) in release mode with -O1 and -O3 and it doesn’t work either. I don’t know, but in my opinion, people are unlikely to release their programs without any optimisation's or in debug mode :)

This is what I see with ANY optimization.

image

This is how it should be (no optimization):

image
NickGerleman commented 3 months ago

This seems really strange to me. E.g. React Native ships to many Apple devices, with optimizations, but has not hit this, in either OSS or Meta internal builds. Same with all the other iOS experiences Meta ships, which usually use Yoga under the hood.

Does -fno-finite-math-only still fix issue for you? That implies something else in build might be causing FP math flags to be set.

Would be interested otherwise in what ends up happening mechanically to cause this. I remember a long while back that Yoga hit MSVC codegen bug, but haven't heard of anything like that happening in a while.

Wiksnet commented 2 months ago

@NickGerleman Yes, -fno-finite-math-onlyhelps.

It is quite strange that the compiler includes this during any optimization. I'm not sure if it's a clang bug or something else. In any case, thank you for your response. I understand that this could indeed be a breaking change. If your team could find a way to avoid this in the future, it would be greatly appreciated.