Skip to content

Remove fastcomp-only EMULATED_FUNCTION_POINTERS setting #11864

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

Merged
merged 2 commits into from
Aug 11, 2020

Conversation

kripken
Copy link
Member

@kripken kripken commented Aug 11, 2020

(some changes best viewed without whitespace)

@kripken kripken requested a review from sbc100 August 11, 2020 21:14
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Hmm.. I thought we had a binaryen pass that does emulated fps? Is that somehow different to this setting? Whats the store for programs that python that need this behaviour?

@@ -1795,19 +1785,6 @@ def test_bigarray(self):
def test_mod_globalstruct(self):
self.do_run_in_out_file_test('tests', 'core', 'test_mod_globalstruct')

@no_wasm_backend('long doubles are f128s in wasm backend')
def test_pystruct(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this test is still useful? It looks like it was disabled not because EMULATED_FUNCTION_POINTERS was missing but because of fp128.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think probably not... it just has some interesting structs in it, taken from python. We test equally interesting things in other places, and it's more of a clang-level feature anyhow. The interesting part of the test was the relocatable + emulated function pointers part which is removed, so I removed the whole test.

@@ -6012,20 +5989,9 @@ def count_relocations():
num_relocs = relocs.count('\n')
return relocs, num_relocs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Huh.. this function looks completely unused both before and after this change...

Copy link
Member Author

Choose a reason for hiding this comment

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

Good eye, removed.

# success!
self.assertContained('Hello, world.', output)
for relocatable in [0, 1]:
for wasm in [0, 1]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yay, one level less!

@kripken
Copy link
Member Author

kripken commented Aug 11, 2020

Yeah, it can be confusing - emulated function pointers is not the same as emulated function pointer casts. The latter is necessary for python and we have a binaryen pass for it, yes.

Getting rid of this setting will remove that confusion!

@sbc100
Copy link
Collaborator

sbc100 commented Aug 11, 2020

Lets try to reference #11860 in all the cleanups.

@kripken kripken merged commit c0470c7 into master Aug 11, 2020
@kripken kripken deleted the no-EMULATED_FUNCTION_POINTERS branch August 11, 2020 22:32
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.

2 participants