Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

engine_xetex: attempt to fix redbox_png test issue #847

Merged
merged 1 commit into from
Dec 7, 2021

Conversation

pkgw
Copy link
Collaborator

@pkgw pkgw commented Dec 7, 2021

We would get results that varied depending on whether we were compiled in debug or release mode. It looks like the root cause is some single-precision math in computing image sizes.

Cf. #845, possibly #776.

We would get results that varied depending on whether we were compiled
in debug or release mode. It looks like the root cause is some
single-precision math in computing image sizes.
@codecov
Copy link

codecov bot commented Dec 7, 2021

Codecov Report

Merging #847 (16f0053) into master (09b1575) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #847   +/-   ##
=======================================
  Coverage   47.07%   47.08%           
=======================================
  Files         146      146           
  Lines       59493    59493           
=======================================
+ Hits        28008    28012    +4     
+ Misses      31485    31481    -4     
Impacted Files Coverage Δ
crates/engine_xetex/xetex/xetex-pic.c 41.49% <100.00%> (ø)
crates/bundles/src/zip.rs 0.00% <0.00%> (-3.71%) ⬇️
src/bin/tectonic/v2cli.rs 48.58% <0.00%> (-0.17%) ⬇️
src/engines/spx2html.rs 0.00% <0.00%> (ø)
crates/io_base/src/filesystem.rs 57.01% <0.00%> (ø)
src/driver.rs 73.40% <0.00%> (+0.20%) ⬆️
crates/bridge_core/src/lib.rs 65.72% <0.00%> (+0.32%) ⬆️
crates/io_base/src/lib.rs 76.27% <0.00%> (+0.42%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09b1575...16f0053. Read the comment docs.

@wisp3rwind
Copy link

This does indeed seem to fix #776 for me.


Random thoughts below this line, because I was quite curious on what the issue might be here, but don't know what the compiler is allowed to do:

/* Tectonic customization: if we use single-precision math, we can
bounds->wd *= 72.27;
* sometimes get numerical results that vary depending on whether we're
bounds->ht *= 72.27;
* compiled in debug or release mode (?!) */

I also don't really understand this... Might this even be a compiler bug, where on --release, get_image_size_in_inches is inlined, and on constant-folding 1 / 72 * 72.27 (is constant-folding this even allowed without fast_math, given that it changes associativity?) something goes wrong with the double -> float type conversions?

@pkgw pkgw merged commit fe67a8d into tectonic-typesetting:master Dec 7, 2021
@pkgw
Copy link
Collaborator Author

pkgw commented Dec 7, 2021

I believe your diagnosis is about correct. In release mode, the behavior changed when I added a printf of bounds->wd between the function call and the multiplication. That implies to me that the two statements were being fused in a way that changed their behavior subtly.

On the other hand, with this fix, the release mode behavior turns out to have been more correct: the behavior of the debug mode changed and I had to update the test files as a result. So I think the inlining actually helped to compiler maintain better precision, while in debug mode the conversion to float must happen "too early" and lose some precision.

@pkgw pkgw deleted the fix-redbox branch December 7, 2021 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants