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

When I build Pd, I get torubles that are probably realated with casting float to int. #927

Closed
porres opened this issue Apr 9, 2020 · 25 comments

Comments

@porres
Copy link
Contributor

porres commented Apr 9, 2020

When I build PD from source on macOS (like the new 0.51-0 test1]), I get this error when I try had the value of 1+e16 for the line number in [text insert]. A related closed issue by thew way: #848

Screen Shot 2020-04-09 at 16 21 30

And here's the patch:
test-text.pd.zip


I'm just doing what I've been doing in order to compile Pd for a few years now, and I do:

./autogen.sh
./configure
make
make app

@umlaeute
Copy link
Contributor

umlaeute commented Apr 9, 2020

unable to reproduce in both the current update/0.50 (f5cdf67) and master ( e2ef815) branches.

EDIT: in Debian GNU/Linux

and no, i wouldn't call this version 0.51 already (nor 51)
since this repository holds a few thousand "versions" of Pd, i guess the proper way to reference a given version is by using the commitish, that is the tag name (e.g .0.50-2) if one exists (since we don't have a release right now, there's no tag yet), or the "sha" hash (e.g. e2ef815)

@porres
Copy link
Contributor Author

porres commented Apr 9, 2020

yeah, I forgot to say, I'm on macOS

@umlaeute
Copy link
Contributor

umlaeute commented Apr 9, 2020

well yeah, your screenshot was giving you away :-)

are you sure that you are using the latest and greatest Pd (which commit?)? because there is code in place that is supposed to specifically handle the problem you are mentioning (4f9c85e)

@porres
Copy link
Contributor Author

porres commented Apr 10, 2020

are you sure that you are using the latest and greatest Pd (which commit?)?

the last very one. I saw this happening in the branch from [knob], then I downloaded the latest source and saw it happening there too... but things work fine in 0.50-2

@Lucarda
Copy link
Contributor

Lucarda commented Apr 10, 2020

I can't reproduce on Windows.

master e2ef815 or update/0.50(20/3/2020).

@umlaeute
Copy link
Contributor

umlaeute commented Apr 10, 2020 via email

@porres
Copy link
Contributor Author

porres commented Apr 10, 2020

e2ef815

@porres
Copy link
Contributor Author

porres commented May 29, 2020

Now that the 0.51-0test is out, I do not see this issue anymore, it seems this is related to the other issue that I was having with one of my externals - see #848

So, the problem seems to be that I'm compiling Pd wrong... I will ask on the pd list what I might be doing wrong

thanks

@porres porres closed this as completed May 29, 2020
@Spacechild1
Copy link
Contributor

-2147483647 is a typical result of signed integer overflow being undefined behavior. BUT the code of [text insert] has the following line

lineno = (x->x_f1 > (double)0x7fffffff ? 0x7fffffff : x->x_f1);

which is supposed to prevent the potential overflow issue... TBH, I have to no idea what's going on... Do you get the same result for [text set]?

@Spacechild1 Spacechild1 reopened this May 29, 2020
@porres porres changed the title [text] issue with high number lines When I build Pd, I get torubles that are probably realated with casting float to int. May 29, 2020
@porres
Copy link
Contributor Author

porres commented May 29, 2020

Ok, I edited my 1st comment and title, so all comments prior to yours can be deleted.

Do you get the same result for [text set]?

Not sure what kind of test you wanted, but this works fine... what I get is that floats are added in the order I send float values (I'm assuming this is expected).

Screen Shot 2020-05-29 at 09 49 40

@claudeha
Copy link
Contributor

x_f1 is float, 0x7fffffff is not representable in float, so the float result of ?: is already out of range, perhaps. I see warnings when compiling, not able to check right now...

@Spacechild1
Copy link
Contributor

x_f1 is float, 0x7fffffff

it's not exactly representable as a float. Floats can certainly be larger than 0x7fffffff. Also note that we cast 0x7fffffff to a double, so x_f1 should be widened appropriately for the comparison.

@Spacechild1
Copy link
Contributor

@porres I need you to compare the behavior of large onset values for both [text set] and [text insert] with the same Pd version (the one you compiled yourself).

@Spacechild1
Copy link
Contributor

Spacechild1 commented May 29, 2020

Ok, I can reproduce the issue when compiling Pd on a Mac with clang. I think Miller cross compiles from Linux with gcc, that might explain the different outcome. Will have a look. And strangely, it only happens with [text insert] and not with [text set], although the code in question is basically the same. Will investigate.

@Spacechild1
Copy link
Contributor

The bug is gone when changing
lineno = (x->x_f1 > (double)0x7fffffff ? 0x7fffffff : x->x_f1);
to
lineno = (x->x_f1 > (double)0x7fffffff ? 0x7fffffff : (int)x->x_f1);

lineno is already an integer, so one would think that explicitly casting x_f1 would be redundant. To me, this looks like a bug in clang... Compiler bugs are very rare, but they do happen after all...

@Spacechild1
Copy link
Contributor

I pushed this "fix" to develop, also for consistency with [text set], but I would like to understand why this seemingly unnecessary cast does the trick... I probably have to look into the assembly...

@Spacechild1
Copy link
Contributor

@porres please test with latest develop and report back.

@porres
Copy link
Contributor Author

porres commented May 29, 2020

@porres please test with latest develop and report back.

IT WORKS :)

Ok... we can close this now I guess, but what about #848 ? It's also an issue that only happens when I compile Pd.

@porres
Copy link
Contributor Author

porres commented May 29, 2020

what about #848 ?

opened a new issue about it... #1046

@claudeha
Copy link
Contributor

x_f1 is float, 0x7fffffff

we cast 0x7fffffff to a double, so x_f1 should be widened appropriately for the comparison.

I meant the other one which was promoted to float by the result type of ?:. Fix looks sensible to me.

@Spacechild1
Copy link
Contributor

I think you're one to something, but I still fail to see where exactly the overflow / undefined behavior occurs...

@claudeha
Copy link
Contributor

x_f1 is float, so 0x7fffffff is promoted from int as ?: has a single result type, but nearest representable float is 0x8000000 which overflows when converted back to int. This is conjecture, looking at assembly would help resolve what is going on.

@Spacechild1
Copy link
Contributor

Spacechild1 commented May 29, 2020

Hmmm. From what I understand, the mantissa is truncated, so the value is effectively rounded down to the next representable value. But I'll do some reading and look at the disassembly.

@Spacechild1
Copy link
Contributor

Spacechild1 commented May 29, 2020

oh, you're right! both clang und GCC decide to round up (float)0x7fffffff at compile time:

https://godbolt.org/z/e2mtg2

Things get more interesting for (int)(float)0x7fffffff:

https://godbolt.org/z/DyKj3j

GCC outputs 2147483647 (as I would have expected), while clang outputs whatever value happens to be in esi (probably because it thinks that casting 0x8000000 back to int is undefined behavior).

@Spacechild1
Copy link
Contributor

As a follow-up, here's what happens when converting an int to a float at runtime with cvtsi2ss:

"When a conversion is inexact, the value returned is rounded according to the rounding control bits in the MXCSR register or the embedded rounding control bits.

https://www.felixcloutier.com/x86/cvtsi2ss

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

No branches or pull requests

5 participants