E2ETest failure: Padding doesn't work for Views on VS 2019, v142 tools, Release x64 #4122

jonthysell · 2020-02-19T18:18:50Z

As part of PR #4101, the measureLayout E2E test is failing on the windows-2019 VM because the View's padding is not being applied.

I've confirmed that it repros on my 19H1+ machine, when building in VS 2019 and the v142 toolset.

We are patching Yoga.cpp in vnext\DeforkingPatches\ReactCommon\yoga\yoga\Yoga.cpp to add an extra measure call, apparently for the benefit of react-native-win32, but it has the side effect of messing up react-native-windows.

Removing that patch fixes the issue for react-native-windows, but may cause other unintended side-effects in react-native-windows, and of course probably messes up react-native-win32.

The text was updated successfully, but these errors were encountered:

jonthysell · 2020-02-19T19:46:56Z

Making things even weirder, this only seems to happen with release bits it's possible this is a debug/release issue.

jonthysell · 2020-03-05T20:15:55Z

This only repros on Release x64, not Release x86.

acoates-ms · 2020-04-14T19:01:11Z

To be clear this is unrelated to the win32 patch -- and just started with the new version of the compiler.

RNTester in playground app in Debug:

RNTester in playground app in Release:

…4585) Improve/refactor e2etest and TreeDumpLibrary in a number of ways: most importantly is that we've had test break but not fail the PR or CI - this is because wdio which we use as a test harness, does not set its exit code on success/failure, which is needed for Azure Pipelines to know whether it should fail the build or not. image Because we were blind to test failures, the outputs have drifted away from the original masters. I'm re-baselining them based on what we have today. Component owners should do a pass on the new masters to make sure nothing looks weird (e.g. ImageRTL still shows FlowDirection=LeftToRight). Filed #4589 to track this. changes the test app UI to provide links to the master and output files, and to diff the two. I wanted to launch vscode in diff mode for that but since we are a UWP app, the only way we can launch a program like that is to use a protocol handler, which vscode doesn't expose for diffing. Filed: Provide a protocol handler for diffing two files. For the time being, we'll copy the command line to launch vscode in diff mode for the two files. Adds the ability for the tree dump library to output json which will allow us to write tests that validate the output from the javascript side Adds logic for optionally output the name of elements. I plan to turn this and json output on soon but wanted to get a baseline PR/CI first. e2etest was crashing because of a bug in Switch: #4596 . Disabled that control in the test app for the time being. Added logic to the pipeline to capture crash dumps when e2etest app crashes Added checks to make sure we don't try to build/run the e2etest app in scenarios where it isn't really supported because the masters would not match (anything other than Release|x64 at 100% DPI). There are bugs tracking these limitations that we must fix before rtm: #4122 and #4619

acoates-ms · 2020-04-16T20:59:32Z

In YGNode::getLeadingPadding

  YGFloatOptional resolvedValue = YGResolveValue(
      YGComputedEdgeValue(
          style_.padding(), leading[axis], CompactValue::ofZero()),
      widthSize);

Is returning 0 in release x64, even when the node.style_.padding_ contains values.

acoates-ms · 2020-04-16T21:14:57Z

You can see the issue in the ASM of YGComputedEdgeValue, where there are calls to ucomiss where one of the parameters would be some unit'd random value, causing the isnan checks within CompactValue to not do the correct thing.

asklar · 2020-04-17T18:55:09Z

…icrosoft#4585) Improve/refactor e2etest and TreeDumpLibrary in a number of ways: most importantly is that we've had test break but not fail the PR or CI - this is because wdio which we use as a test harness, does not set its exit code on success/failure, which is needed for Azure Pipelines to know whether it should fail the build or not. image Because we were blind to test failures, the outputs have drifted away from the original masters. I'm re-baselining them based on what we have today. Component owners should do a pass on the new masters to make sure nothing looks weird (e.g. ImageRTL still shows FlowDirection=LeftToRight). Filed microsoft#4589 to track this. changes the test app UI to provide links to the master and output files, and to diff the two. I wanted to launch vscode in diff mode for that but since we are a UWP app, the only way we can launch a program like that is to use a protocol handler, which vscode doesn't expose for diffing. Filed: Provide a protocol handler for diffing two files. For the time being, we'll copy the command line to launch vscode in diff mode for the two files. Adds the ability for the tree dump library to output json which will allow us to write tests that validate the output from the javascript side Adds logic for optionally output the name of elements. I plan to turn this and json output on soon but wanted to get a baseline PR/CI first. e2etest was crashing because of a bug in Switch: microsoft#4596 . Disabled that control in the test app for the time being. Added logic to the pipeline to capture crash dumps when e2etest app crashes Added checks to make sure we don't try to build/run the e2etest app in scenarios where it isn't really supported because the masters would not match (anything other than Release|x64 at 100% DPI). There are bugs tracking these limitations that we must fix before rtm: microsoft#4122 and microsoft#4619

YGValue, MSVC does define NAN) to __builtin_nanf("0"), which generates a quiet NaN. The problem with NAN is that corecrt_math.h defines it as: Which *seems* fine! However, because we are compiling with /fp:precise (see: microsoft#4122 for more details), that multiplication actually happens at runtime. And, unfortunately, we're sometimes seeing MXCSR set to round down (i.e., the _MM_ROUND_DOWN flag is set)), which, combined with the SSE instructions that happen to run here (specifically, cvtpd2ps), cause NAN, and thus YGUndefined, to yield a value of 0! What happens is that cvtpd2ps, when rounding down, converts 1#.INF in double-precision not to 1#.INF in single-precision, but instead to FLT_MAX!!! And, of course, FLT_MAX*0 is 0, not qNaN. Oops. Anyways, using __builting_nanf("0") just loads 0x7FC0000 into xmm0, rather than doing any math, so, even with /fp:precise + bad rounding modesm, YGUndefined is, in fact, a quiet NaN.

microsoft#8675. We use this flag to address microsoft#4122, but in my manual testing, as well by inspecting the disassembly that seemed to be responsible for issue 4122, this workaround is no longer necessary.

* Change the definition of YGUndefined from NAN (contra the comment in YGValue, MSVC does define NAN) to __builtin_nanf("0"), which generates a quiet NaN. The problem with NAN is that corecrt_math.h defines it as: Which *seems* fine! However, because we are compiling with /fp:precise (see: #4122 for more details), that multiplication actually happens at runtime. And, unfortunately, we're sometimes seeing MXCSR set to round down (i.e., the_MM_ROUND_DOWN flag is set)), which, combined with th SSE instructions that happen to run here (specifically, cvtpd2ps), cause NAN, and thus YGUndefined, to yield a value of 0! What happens is that cvtpd2ps, when rounding down, converts 1#.INF in double-precision not to 1#.INF in single-precision, but instead to FLT_MAX!!! And, of course, FLT_MAX*0 is 0, not qNaN. Oops. Anyways, using __builting_nanf("0") just loads 0x7FC0000 into xmm0, rather than doing any math, so, even with /fp:precise + bad rounding modes, YGUndefined is, in fact, a quiet NaN. * Revert "Change the definition of YGUndefined from NAN (contra the comment in" This reverts commit 3fb68a2. Rather than trying to fork YGValue.h, we'll address the underlying problem by not using /fp:strict for yoga.cpp * Compiling yoga.cpp with /fp:strict causes #8675. We use this flag to address #4122, but in my manual testing, as well by inspecting the disassembly that seemed to be responsible for issue 4122, this workaround is no longer necessary. * Patch yoga to use better definition for YGUndefined * Change files Co-authored-by: dannyvv <dannyvv@microsoft.com> Co-authored-by: Andrew Coates <30809111+acoates-ms@users.noreply.github.com>

* Change the definition of YGUndefined from NAN (contra the comment in YGValue, MSVC does define NAN) to __builtin_nanf("0"), which generates a quiet NaN. The problem with NAN is that corecrt_math.h defines it as: Which *seems* fine! However, because we are compiling with /fp:precise (see: microsoft#4122 for more details), that multiplication actually happens at runtime. And, unfortunately, we're sometimes seeing MXCSR set to round down (i.e., the_MM_ROUND_DOWN flag is set)), which, combined with th SSE instructions that happen to run here (specifically, cvtpd2ps), cause NAN, and thus YGUndefined, to yield a value of 0! What happens is that cvtpd2ps, when rounding down, converts 1#.INF in double-precision not to 1#.INF in single-precision, but instead to FLT_MAX!!! And, of course, FLT_MAX*0 is 0, not qNaN. Oops. Anyways, using __builting_nanf("0") just loads 0x7FC0000 into xmm0, rather than doing any math, so, even with /fp:precise + bad rounding modes, YGUndefined is, in fact, a quiet NaN. * Revert "Change the definition of YGUndefined from NAN (contra the comment in" This reverts commit 3fb68a2. Rather than trying to fork YGValue.h, we'll address the underlying problem by not using /fp:strict for yoga.cpp * Compiling yoga.cpp with /fp:strict causes microsoft#8675. We use this flag to address microsoft#4122, but in my manual testing, as well by inspecting the disassembly that seemed to be responsible for issue 4122, this workaround is no longer necessary. * Patch yoga to use better definition for YGUndefined * Change files Co-authored-by: dannyvv <dannyvv@microsoft.com> Co-authored-by: Andrew Coates <30809111+acoates-ms@users.noreply.github.com>

* [0.64] Fix ARM64 layout issues in Office (#8689) * Change the definition of YGUndefined from NAN (contra the comment in YGValue, MSVC does define NAN) to __builtin_nanf("0"), which generates a quiet NaN. The problem with NAN is that corecrt_math.h defines it as: Which *seems* fine! However, because we are compiling with /fp:precise (see: #4122 for more details), that multiplication actually happens at runtime. And, unfortunately, we're sometimes seeing MXCSR set to round down (i.e., the_MM_ROUND_DOWN flag is set)), which, combined with th SSE instructions that happen to run here (specifically, cvtpd2ps), cause NAN, and thus YGUndefined, to yield a value of 0! What happens is that cvtpd2ps, when rounding down, converts 1#.INF in double-precision not to 1#.INF in single-precision, but instead to FLT_MAX!!! And, of course, FLT_MAX*0 is 0, not qNaN. Oops. Anyways, using __builting_nanf("0") just loads 0x7FC0000 into xmm0, rather than doing any math, so, even with /fp:precise + bad rounding modes, YGUndefined is, in fact, a quiet NaN. * Revert "Change the definition of YGUndefined from NAN (contra the comment in" This reverts commit 3fb68a2. Rather than trying to fork YGValue.h, we'll address the underlying problem by not using /fp:strict for yoga.cpp * Compiling yoga.cpp with /fp:strict causes #8675. We use this flag to address #4122, but in my manual testing, as well by inspecting the disassembly that seemed to be responsible for issue 4122, this workaround is no longer necessary. * Patch yoga to use better definition for YGUndefined * Change files Co-authored-by: dannyvv <dannyvv@microsoft.com> Co-authored-by: Andrew Coates <30809111+acoates-ms@users.noreply.github.com> * Fix change file * react-native-platform-override upgrade Co-authored-by: Harold Pratt <38818465+htpiv@users.noreply.github.com> Co-authored-by: dannyvv <dannyvv@microsoft.com>

* [0.64] Fix ARM64 layout issues in Office (#8689) * Change the definition of YGUndefined from NAN (contra the comment in YGValue, MSVC does define NAN) to __builtin_nanf("0"), which generates a quiet NaN. The problem with NAN is that corecrt_math.h defines it as: Which *seems* fine! However, because we are compiling with /fp:precise (see: #4122 for more details), that multiplication actually happens at runtime. And, unfortunately, we're sometimes seeing MXCSR set to round down (i.e., the_MM_ROUND_DOWN flag is set)), which, combined with th SSE instructions that happen to run here (specifically, cvtpd2ps), cause NAN, and thus YGUndefined, to yield a value of 0! What happens is that cvtpd2ps, when rounding down, converts 1#.INF in double-precision not to 1#.INF in single-precision, but instead to FLT_MAX!!! And, of course, FLT_MAX*0 is 0, not qNaN. Oops. Anyways, using __builting_nanf("0") just loads 0x7FC0000 into xmm0, rather than doing any math, so, even with /fp:precise + bad rounding modes, YGUndefined is, in fact, a quiet NaN. * Revert "Change the definition of YGUndefined from NAN (contra the comment in" This reverts commit 3fb68a2. Rather than trying to fork YGValue.h, we'll address the underlying problem by not using /fp:strict for yoga.cpp * Compiling yoga.cpp with /fp:strict causes #8675. We use this flag to address #4122, but in my manual testing, as well by inspecting the disassembly that seemed to be responsible for issue 4122, this workaround is no longer necessary. * Patch yoga to use better definition for YGUndefined * Change files Co-authored-by: dannyvv <dannyvv@microsoft.com> Co-authored-by: Andrew Coates <30809111+acoates-ms@users.noreply.github.com> * [0.64] Fix ARM64 layout issues in Office (#8689) * Change the definition of YGUndefined from NAN (contra the comment in YGValue, MSVC does define NAN) to __builtin_nanf("0"), which generates a quiet NaN. The problem with NAN is that corecrt_math.h defines it as: Which *seems* fine! However, because we are compiling with /fp:precise (see: #4122 for more details), that multiplication actually happens at runtime. And, unfortunately, we're sometimes seeing MXCSR set to round down (i.e., the_MM_ROUND_DOWN flag is set)), which, combined with th SSE instructions that happen to run here (specifically, cvtpd2ps), cause NAN, and thus YGUndefined, to yield a value of 0! What happens is that cvtpd2ps, when rounding down, converts 1#.INF in double-precision not to 1#.INF in single-precision, but instead to FLT_MAX!!! And, of course, FLT_MAX*0 is 0, not qNaN. Oops. Anyways, using __builting_nanf("0") just loads 0x7FC0000 into xmm0, rather than doing any math, so, even with /fp:precise + bad rounding modes, YGUndefined is, in fact, a quiet NaN. * Revert "Change the definition of YGUndefined from NAN (contra the comment in" This reverts commit 3fb68a2. Rather than trying to fork YGValue.h, we'll address the underlying problem by not using /fp:strict for yoga.cpp * Compiling yoga.cpp with /fp:strict causes #8675. We use this flag to address #4122, but in my manual testing, as well by inspecting the disassembly that seemed to be responsible for issue 4122, this workaround is no longer necessary. * Patch yoga to use better definition for YGUndefined * Change files Co-authored-by: dannyvv <dannyvv@microsoft.com> Co-authored-by: Andrew Coates <30809111+acoates-ms@users.noreply.github.com> * Remove duplicate in overrides.json Co-authored-by: Harold Pratt <38818465+htpiv@users.noreply.github.com> Co-authored-by: dannyvv <dannyvv@microsoft.com>

* [0.64] Fix ARM64 layout issues in Office (microsoft#8689) * Change the definition of YGUndefined from NAN (contra the comment in YGValue, MSVC does define NAN) to __builtin_nanf("0"), which generates a quiet NaN. The problem with NAN is that corecrt_math.h defines it as: Which *seems* fine! However, because we are compiling with /fp:precise (see: microsoft#4122 for more details), that multiplication actually happens at runtime. And, unfortunately, we're sometimes seeing MXCSR set to round down (i.e., the_MM_ROUND_DOWN flag is set)), which, combined with th SSE instructions that happen to run here (specifically, cvtpd2ps), cause NAN, and thus YGUndefined, to yield a value of 0! What happens is that cvtpd2ps, when rounding down, converts 1#.INF in double-precision not to 1#.INF in single-precision, but instead to FLT_MAX!!! And, of course, FLT_MAX*0 is 0, not qNaN. Oops. Anyways, using __builting_nanf("0") just loads 0x7FC0000 into xmm0, rather than doing any math, so, even with /fp:precise + bad rounding modes, YGUndefined is, in fact, a quiet NaN. * Revert "Change the definition of YGUndefined from NAN (contra the comment in" This reverts commit 3fb68a2. Rather than trying to fork YGValue.h, we'll address the underlying problem by not using /fp:strict for yoga.cpp * Compiling yoga.cpp with /fp:strict causes microsoft#8675. We use this flag to address microsoft#4122, but in my manual testing, as well by inspecting the disassembly that seemed to be responsible for issue 4122, this workaround is no longer necessary. * Patch yoga to use better definition for YGUndefined * Change files Co-authored-by: dannyvv <dannyvv@microsoft.com> Co-authored-by: Andrew Coates <30809111+acoates-ms@users.noreply.github.com> * Fix change file * react-native-platform-override upgrade Co-authored-by: Harold Pratt <38818465+htpiv@users.noreply.github.com> Co-authored-by: dannyvv <dannyvv@microsoft.com>

* Stop compiling yoga.cpp with /fp:strict. Doing so caused layout issues if Yoga code ran with the processor's rounding mode set to round down, due to NAN being defined in math.h as: ``` (float)(INFINITY * 0.0f) ``` Which macro-expands to: ``` (float)(((float)(1e+300 * 1e+300)) * 0.0f) ``` Which evaluates as follows: ``` (float)(((float)(inf.double)) * 0.0f) (float)(FLT_MAX * 0.0f) // Casting an infinite double to a float yields // FLT_MAX! (float)0.0f ``` Oops. Also, /fp:strict is slightly more pessimistic & slower... not that anyone should be running Yoga layout in a tight loop or anything! But free perf is always nice. I manually inspected the disassembly code that was identified as causing #4122, and I did not find any issues anymore. Unfortunately, while the exact issue tracked by #4122 was fixed, it turned out that different code was still compiling incorrectly. It appears that recent versions of MSVC are generating bad code when compiling Yoga's `CompactValue` class; specifically, it seems that the undefined behavior in `CompactValue::isUndefined()` is leading to bad codegen. That method has undefined behavior because it accesses two members of a union, one a `float`, the other a `uint32_t`, only one of which can be active. The resulting bad codegen in MSVC is that (on amd64; I did not test other architectures), when `isUndefined()` calls `isnan()` on the `float` union member, the compiler emits code to do a `ucomiss` between an XMM register containing the union member and another XMM register whose contents are indeterminate (they're whatever the caller happens to have left behind in the XMM register). Oops! The effect is that, sometimes, paddings and borders are ignored. We're reporting this bad codegen to the MSVC team, but at the same time, we're going to put a fix into RNW, and also send a PR to the Yoga team to, on platforms with C++20, use bit_cast, and otherwise use the Officially Supported mechanism for type-punning of using memcpy (which, I confirmed on godbolt, clang, GCC, and MSVC all know to optimize down to a simple assignment). * Remove YGValue.h, since the forked change to how YGUndefined is defined is no longer needed. * Add CompactValue.h to overrides.json Co-authored-by: Danny van Velzen <dannyvv@microsoft.com>

* Stop compiling yoga.cpp with /fp:strict. Doing so caused layout issues if Yoga code ran with the processor's rounding mode set to round down, due to NAN being defined in math.h as: ``` (float)(INFINITY * 0.0f) ``` Which macro-expands to: ``` (float)(((float)(1e+300 * 1e+300)) * 0.0f) ``` Which evaluates as follows: ``` (float)(((float)(inf.double)) * 0.0f) (float)(FLT_MAX * 0.0f) // Casting an infinite double to a float yields // FLT_MAX! (float)0.0f ``` Oops. Also, /fp:strict is slightly more pessimistic & slower... not that anyone should be running Yoga layout in a tight loop or anything! But free perf is always nice. I manually inspected the disassembly code that was identified as causing microsoft#4122, and I did not find any issues anymore. Unfortunately, while the exact issue tracked by microsoft#4122 was fixed, it turned out that different code was still compiling incorrectly. It appears that recent versions of MSVC are generating bad code when compiling Yoga's `CompactValue` class; specifically, it seems that the undefined behavior in `CompactValue::isUndefined()` is leading to bad codegen. That method has undefined behavior because it accesses two members of a union, one a `float`, the other a `uint32_t`, only one of which can be active. The resulting bad codegen in MSVC is that (on amd64; I did not test other architectures), when `isUndefined()` calls `isnan()` on the `float` union member, the compiler emits code to do a `ucomiss` between an XMM register containing the union member and another XMM register whose contents are indeterminate (they're whatever the caller happens to have left behind in the XMM register). Oops! The effect is that, sometimes, paddings and borders are ignored. We're reporting this bad codegen to the MSVC team, but at the same time, we're going to put a fix into RNW, and also send a PR to the Yoga team to, on platforms with C++20, use bit_cast, and otherwise use the Officially Supported mechanism for type-punning of using memcpy (which, I confirmed on godbolt, clang, GCC, and MSVC all know to optimize down to a simple assignment). * Remove YGValue.h, since the forked change to how YGUndefined is defined is no longer needed. * Add CompactValue.h to overrides.json Co-authored-by: Danny van Velzen <dannyvv@microsoft.com>

* Stop using /fp:strict to build yoga.cpp (#8725) * Stop compiling yoga.cpp with /fp:strict. Doing so caused layout issues if Yoga code ran with the processor's rounding mode set to round down, due to NAN being defined in math.h as: ``` (float)(INFINITY * 0.0f) ``` Which macro-expands to: ``` (float)(((float)(1e+300 * 1e+300)) * 0.0f) ``` Which evaluates as follows: ``` (float)(((float)(inf.double)) * 0.0f) (float)(FLT_MAX * 0.0f) // Casting an infinite double to a float yields // FLT_MAX! (float)0.0f ``` Oops. Also, /fp:strict is slightly more pessimistic & slower... not that anyone should be running Yoga layout in a tight loop or anything! But free perf is always nice. I manually inspected the disassembly code that was identified as causing #4122, and I did not find any issues anymore. Unfortunately, while the exact issue tracked by #4122 was fixed, it turned out that different code was still compiling incorrectly. It appears that recent versions of MSVC are generating bad code when compiling Yoga's `CompactValue` class; specifically, it seems that the undefined behavior in `CompactValue::isUndefined()` is leading to bad codegen. That method has undefined behavior because it accesses two members of a union, one a `float`, the other a `uint32_t`, only one of which can be active. The resulting bad codegen in MSVC is that (on amd64; I did not test other architectures), when `isUndefined()` calls `isnan()` on the `float` union member, the compiler emits code to do a `ucomiss` between an XMM register containing the union member and another XMM register whose contents are indeterminate (they're whatever the caller happens to have left behind in the XMM register). Oops! The effect is that, sometimes, paddings and borders are ignored. We're reporting this bad codegen to the MSVC team, but at the same time, we're going to put a fix into RNW, and also send a PR to the Yoga team to, on platforms with C++20, use bit_cast, and otherwise use the Officially Supported mechanism for type-punning of using memcpy (which, I confirmed on godbolt, clang, GCC, and MSVC all know to optimize down to a simple assignment). * Remove YGValue.h, since the forked change to how YGUndefined is defined is no longer needed. * Add CompactValue.h to overrides.json Co-authored-by: Danny van Velzen <dannyvv@microsoft.com> * fix change file * fix overrides Co-authored-by: Danny van Velzen <dannyvv@microsoft.com> Co-authored-by: Andrew Coates <30809111+acoates-ms@users.noreply.github.com>

* Stop using /fp:strict to build yoga.cpp (#8725) * Stop compiling yoga.cpp with /fp:strict. Doing so caused layout issues if Yoga code ran with the processor's rounding mode set to round down, due to NAN being defined in math.h as: ``` (float)(INFINITY * 0.0f) ``` Which macro-expands to: ``` (float)(((float)(1e+300 * 1e+300)) * 0.0f) ``` Which evaluates as follows: ``` (float)(((float)(inf.double)) * 0.0f) (float)(FLT_MAX * 0.0f) // Casting an infinite double to a float yields // FLT_MAX! (float)0.0f ``` Oops. Also, /fp:strict is slightly more pessimistic & slower... not that anyone should be running Yoga layout in a tight loop or anything! But free perf is always nice. I manually inspected the disassembly code that was identified as causing #4122, and I did not find any issues anymore. Unfortunately, while the exact issue tracked by #4122 was fixed, it turned out that different code was still compiling incorrectly. It appears that recent versions of MSVC are generating bad code when compiling Yoga's `CompactValue` class; specifically, it seems that the undefined behavior in `CompactValue::isUndefined()` is leading to bad codegen. That method has undefined behavior because it accesses two members of a union, one a `float`, the other a `uint32_t`, only one of which can be active. The resulting bad codegen in MSVC is that (on amd64; I did not test other architectures), when `isUndefined()` calls `isnan()` on the `float` union member, the compiler emits code to do a `ucomiss` between an XMM register containing the union member and another XMM register whose contents are indeterminate (they're whatever the caller happens to have left behind in the XMM register). Oops! The effect is that, sometimes, paddings and borders are ignored. We're reporting this bad codegen to the MSVC team, but at the same time, we're going to put a fix into RNW, and also send a PR to the Yoga team to, on platforms with C++20, use bit_cast, and otherwise use the Officially Supported mechanism for type-punning of using memcpy (which, I confirmed on godbolt, clang, GCC, and MSVC all know to optimize down to a simple assignment). * Remove YGValue.h, since the forked change to how YGUndefined is defined is no longer needed. * Add CompactValue.h to overrides.json Co-authored-by: Danny van Velzen <dannyvv@microsoft.com> * Update commit hash to match Yoga version used in 0.67 * Fix change file to be a "patch" Co-authored-by: Danny van Velzen <dannyvv@microsoft.com> Co-authored-by: Andrew Coates <30809111+acoates-ms@users.noreply.github.com>

* Stop using /fp:strict to build yoga.cpp (#8725) * Stop compiling yoga.cpp with /fp:strict. Doing so caused layout issues if Yoga code ran with the processor's rounding mode set to round down, due to NAN being defined in math.h as: ``` (float)(INFINITY * 0.0f) ``` Which macro-expands to: ``` (float)(((float)(1e+300 * 1e+300)) * 0.0f) ``` Which evaluates as follows: ``` (float)(((float)(inf.double)) * 0.0f) (float)(FLT_MAX * 0.0f) // Casting an infinite double to a float yields // FLT_MAX! (float)0.0f ``` Oops. Also, /fp:strict is slightly more pessimistic & slower... not that anyone should be running Yoga layout in a tight loop or anything! But free perf is always nice. I manually inspected the disassembly code that was identified as causing #4122, and I did not find any issues anymore. Unfortunately, while the exact issue tracked by #4122 was fixed, it turned out that different code was still compiling incorrectly. It appears that recent versions of MSVC are generating bad code when compiling Yoga's `CompactValue` class; specifically, it seems that the undefined behavior in `CompactValue::isUndefined()` is leading to bad codegen. That method has undefined behavior because it accesses two members of a union, one a `float`, the other a `uint32_t`, only one of which can be active. The resulting bad codegen in MSVC is that (on amd64; I did not test other architectures), when `isUndefined()` calls `isnan()` on the `float` union member, the compiler emits code to do a `ucomiss` between an XMM register containing the union member and another XMM register whose contents are indeterminate (they're whatever the caller happens to have left behind in the XMM register). Oops! The effect is that, sometimes, paddings and borders are ignored. We're reporting this bad codegen to the MSVC team, but at the same time, we're going to put a fix into RNW, and also send a PR to the Yoga team to, on platforms with C++20, use bit_cast, and otherwise use the Officially Supported mechanism for type-punning of using memcpy (which, I confirmed on godbolt, clang, GCC, and MSVC all know to optimize down to a simple assignment). * Remove YGValue.h, since the forked change to how YGUndefined is defined is no longer needed. * Add CompactValue.h to overrides.json Co-authored-by: Danny van Velzen <dannyvv@microsoft.com> * Fix baseHash of CompactValue.h * Make change type 'patch' instead of 'prerelease' * Remove accidentally added overrides * Fix baseHash for CompactValue.h Co-authored-by: Danny van Velzen <dannyvv@microsoft.com>

* Stop using /fp:strict to build yoga.cpp (#8725) * Stop compiling yoga.cpp with /fp:strict. Doing so caused layout issues if Yoga code ran with the processor's rounding mode set to round down, due to NAN being defined in math.h as: ``` (float)(INFINITY * 0.0f) ``` Which macro-expands to: ``` (float)(((float)(1e+300 * 1e+300)) * 0.0f) ``` Which evaluates as follows: ``` (float)(((float)(inf.double)) * 0.0f) (float)(FLT_MAX * 0.0f) // Casting an infinite double to a float yields // FLT_MAX! (float)0.0f ``` Oops. Also, /fp:strict is slightly more pessimistic & slower... not that anyone should be running Yoga layout in a tight loop or anything! But free perf is always nice. I manually inspected the disassembly code that was identified as causing #4122, and I did not find any issues anymore. Unfortunately, while the exact issue tracked by #4122 was fixed, it turned out that different code was still compiling incorrectly. It appears that recent versions of MSVC are generating bad code when compiling Yoga's `CompactValue` class; specifically, it seems that the undefined behavior in `CompactValue::isUndefined()` is leading to bad codegen. That method has undefined behavior because it accesses two members of a union, one a `float`, the other a `uint32_t`, only one of which can be active. The resulting bad codegen in MSVC is that (on amd64; I did not test other architectures), when `isUndefined()` calls `isnan()` on the `float` union member, the compiler emits code to do a `ucomiss` between an XMM register containing the union member and another XMM register whose contents are indeterminate (they're whatever the caller happens to have left behind in the XMM register). Oops! The effect is that, sometimes, paddings and borders are ignored. We're reporting this bad codegen to the MSVC team, but at the same time, we're going to put a fix into RNW, and also send a PR to the Yoga team to, on platforms with C++20, use bit_cast, and otherwise use the Officially Supported mechanism for type-punning of using memcpy (which, I confirmed on godbolt, clang, GCC, and MSVC all know to optimize down to a simple assignment). * Remove YGValue.h, since the forked change to how YGUndefined is defined is no longer needed. * Add CompactValue.h to overrides.json Co-authored-by: Danny van Velzen <dannyvv@microsoft.com> * Update baseHash of CompactValue.h * Make change type 'patch' instead of 'prerelease' * Change files * Revert "Update baseHash of CompactValue.h" This reverts commit c11c602. * One more time computing the baseHash... Co-authored-by: Danny van Velzen <dannyvv@microsoft.com>

C++ does not, pedantically, allow the use of unions for type-punning in the way that C does. Most compilers, in practice, do support it; however, recent versions of MSVC appear to have a bug that cause bad code to be generated due to this U.B. (see: https://developercommunity.visualstudio.com/t/Bad-code-generated-for-std::isnan-compil/10082631). This led to a series of issues in the react-native-windows project, see: * microsoft/react-native-windows#4122 * microsoft/react-native-windows#8675 In C++20, the `<bit>` header and `bit_cast` function provide a pleasant API for type-punning. Since C++20 is not universally available, if the feature-test macro for `bit_cast` is not defined, memcpy is used instead.

…ion for type-punning (#1154) Summary: C++ does not, pedantically, allow the use of unions for type-punning in the way that C does. Most compilers, in practice, do support it; however, recent versions of MSVC appear to have a bug that cause bad code to be generated due to this U.B. (see: https://developercommunity.visualstudio.com/t/Bad-code-generated-for-std::isnan-compil/10082631). This led to a series of issues in the react-native-windows project, see: * microsoft/react-native-windows#4122 * microsoft/react-native-windows#8675 In C++20, the `<bit>` header and `bit_cast` function provide a pleasant API for type-punning. Since C++20 is not universally available, if the feature-test macro for `bit_cast` is not defined, memcpy is used instead. Pull Request resolved: #1154 Reviewed By: Andrey-Mishanin Differential Revision: D38082048 Pulled By: rozele fbshipit-source-id: a5da08cfb7d4296c725fb44871c55dbb12dc71e5

…ion for type-punning (#1154) Summary: C++ does not, pedantically, allow the use of unions for type-punning in the way that C does. Most compilers, in practice, do support it; however, recent versions of MSVC appear to have a bug that cause bad code to be generated due to this U.B. (see: https://developercommunity.visualstudio.com/t/Bad-code-generated-for-std::isnan-compil/10082631). This led to a series of issues in the react-native-windows project, see: * microsoft/react-native-windows#4122 * microsoft/react-native-windows#8675 In C++20, the `<bit>` header and `bit_cast` function provide a pleasant API for type-punning. Since C++20 is not universally available, if the feature-test macro for `bit_cast` is not defined, memcpy is used instead. X-link: facebook/yoga#1154 Reviewed By: Andrey-Mishanin Differential Revision: D38082048 Pulled By: rozele fbshipit-source-id: a5da08cfb7d4296c725fb44871c55dbb12dc71e5

jonthysell added bug vnext labels Feb 19, 2020

ghost added the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Feb 19, 2020

harinikmsft removed the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Feb 20, 2020

harinikmsft assigned jonthysell Feb 20, 2020

harinikmsft added this to the MVP+ (vNext M5) milestone Feb 20, 2020

jonthysell changed the title ~~E2ETest failure: Padding doesn't work for Views on Windows 2019~~ E2ETest failure: Padding doesn't work for Views on VS 2019, v142 tools, Release x64 Mar 5, 2020

chrisglein removed the vnext label Mar 18, 2020

jonthysell mentioned this issue Apr 2, 2020

Switching to VS 2019 and v142 tools #4101

Merged

chrisglein added the Area: Tests label Apr 13, 2020

chrisglein mentioned this issue Apr 13, 2020

E2E have been failing since around 4/2-4/3 #4579

Closed

chrisglein added the must-have p1 label Apr 14, 2020

acoates-ms mentioned this issue Apr 16, 2020

Scaling is wrong in RNW, possible Yoga issue #4619

Closed

asklar mentioned this issue Apr 16, 2020

Fix E2E Tests, add CI break on test failure, improve TreeDumpLibrary #4585

Merged

chrisglein mentioned this issue Apr 16, 2020

Verify that the updated masters are correct #4589

Closed

acoates-ms mentioned this issue Apr 17, 2020

Fix issue with yoga in X64 Release builds #4628

Merged

ghost closed this as completed in #4628 Apr 17, 2020

This was referenced Apr 19, 2020

Disable break for 4122 #4635

Closed

Remove break for 4122 and reenable DManip test #4636

Merged

This was referenced Sep 21, 2021

[0.63] Fix ARM64 layout issues in Office #8676

Merged

YGUndefined has wrong value when the CPU rounding mode is set to round down #8675

Closed

htpiv mentioned this issue Sep 28, 2021

Stop using /fp:strict to build yoga.cpp #8725

Merged

htpiv mentioned this issue Jun 20, 2022

[0.66] Stop using /fp:strict to build yoga.cpp #10170

Merged

This was referenced Jun 21, 2022

[0.67] Stop using /fp:strict to build yoga.cpp #10176

Merged

[0.68] Stop using /fp:strict to build yoga.cpp #10178

Merged

[0.69] Stop using /fp:strict to build yoga.cpp #10179

Merged

htpiv mentioned this issue Jul 18, 2022

Rewrite CompactValue to avoid undefined behavior from the use of a union for type-punning facebook/yoga#1154

Closed

This issue was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

E2ETest failure: Padding doesn't work for Views on VS 2019, v142 tools, Release x64 #4122

E2ETest failure: Padding doesn't work for Views on VS 2019, v142 tools, Release x64 #4122

jonthysell commented Feb 19, 2020

jonthysell commented Feb 19, 2020 •

edited

Loading

jonthysell commented Mar 5, 2020

acoates-ms commented Apr 14, 2020 •

edited

Loading

acoates-ms commented Apr 16, 2020 •

edited

Loading

acoates-ms commented Apr 16, 2020 •

edited

Loading

asklar commented Apr 17, 2020

E2ETest failure: Padding doesn't work for Views on VS 2019, v142 tools, Release x64 #4122

E2ETest failure: Padding doesn't work for Views on VS 2019, v142 tools, Release x64 #4122

Comments

jonthysell commented Feb 19, 2020

jonthysell commented Feb 19, 2020 • edited Loading

jonthysell commented Mar 5, 2020

acoates-ms commented Apr 14, 2020 • edited Loading

acoates-ms commented Apr 16, 2020 • edited Loading

acoates-ms commented Apr 16, 2020 • edited Loading

asklar commented Apr 17, 2020

jonthysell commented Feb 19, 2020 •

edited

Loading

acoates-ms commented Apr 14, 2020 •

edited

Loading

acoates-ms commented Apr 16, 2020 •

edited

Loading

acoates-ms commented Apr 16, 2020 •

edited

Loading