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.

Sign up for GitHub

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

Jump to bottom

when tonumber() is recorded as nil, specialize to the failing ctype #1

Open
wants to merge 1 commit into
base: v2.1
Choose a base branch
from
Open

when tonumber() is recorded as nil, specialize to the failing ctype #1

wants to merge 1 commit into from

Conversation

javierguerragiraldez
Copy link
Member

bug report LuaJIT#408. a function was traced where a tonumber() couldn't convert a struct, so it returned constant nil (and optimized away). When called again with an int cdata, it just executed without checking the ctype.

this patch keeps the ctype guard even in the nil case.

@javierguerragiraldez
Copy link
Member Author

testcase:

local ffi = require 'ffi'
local function isnumberlike(x)
	local is
	for i=1,100 do
		is = tonumber(x)
	end
	return not not is
end

print("isnumberlike(struct)?", isnumberlike(ffi.new('struct {}')))
print("isnumberlike(int64_t)?", isnumberlike(9LL))

expected:
isnumberlike(struct)? false
isnumberlike(int64_t)? true

@mraleph
Copy link

mraleph commented Apr 8, 2018

LGTM

@javierguerragiraldez javierguerragiraldez added the bug Something isn't working label Apr 8, 2018
@mraleph mraleph self-requested a review April 9, 2018 10:58
kostja
kostja suggested changes Apr 11, 2018
View reviewed changes
src/lj_crecord.c
@@ -1879,6 +1879,8 @@ void LJ_FASTCALL lj_crecord_tonumber(jit_State *J, RecordFFData *rd)
d = ctype_get(cts, CTID_DOUBLE);
J->base[0] = crec_ct_tv(J, d, 0, J->base[0], &rd->argv[0]);
} else {
/* specialize to the ctype that couldn't be converted */
argv2cdata(J, J->base[0], &rd->argv[0]);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding a test case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kostja kostja kostja requested changes

@mraleph mraleph mraleph approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@javierguerragiraldez @mraleph @kostja