[Memory64] Replace int (used as ptr) with long in tests by aardappel · Pull Request #14613 · emscripten-core/emscripten

aardappel · 2021-07-09T20:55:35Z

There are a lot of places where pointers get cast to int, and to prepare for further Memory64 changes, those are changed to long.
I chose long since it is most similar to the existing int, instead of e.g intptr_t, since long is more universal (there are many cases where the actual data stored is not a pointer).
Since under wasm32 int and long are equal, this should in theory be a NFC for wasm32, making it easier to review.
It also allows #12869 to be smaller

kripken

In general this sounds good. I don't have a better idea than long here myself. But curious if others have ideas.

To be sure, the title says "replace int with long", but this only replaces the ints used as pointers, as implied in the description?

lgtm % those + the comment

kripken · 2021-07-09T23:34:48Z


 int main() {
-  return (int)memchr("hello", 'z', 7);
+  return (int)(long)memchr("hello", 'z', 7);


Why do two casts here? (happens in a few more tests too)

because casting a 64-bit pointer straight to int results in a warning/error

I see, thanks, sgtm.

I would use intptr_t here .. it seems like just what that type is designed for .. and it should work with just a single cast I think.

@sbc100 how? intptr_t is 64-bit.. that won't let itself be passed to an int context without warning/error.

sbc100 · 2021-07-11T17:42:38Z


 bool __attribute__((noinline)) DOS_Device::Read(unsigned char * data,unsigned short * size) {
-    printf("DOS_Device::Read (this = %i)\n", (int)this);
+    printf("DOS_Device::Read (this = %ld)\n", (long)this);


This should just be %p with no cast, no? Same wherever we want to printf a pointer.

Yup, that be better.. I was just trying to fix the existing style which seems to like to cast to int.
411 results for %ld (most of which "pointers", only 115 of which from PR) - might want to leave this improvement for another PR though.

Maybe lets land that first? That would make this PR a lot smaller. I can have a go at it if you like?

Wouldn't that produce a ton of conflicts with this PR?
I don't think we should mix trying to make things 64-bit compatible with general large scale cleanup of tests. I am sure if you go look at the tests you can find a lot of things to clean up.

I think its good to do such cleanups when it becomes apparent that they are useful/needed. In this case doing the cleanup ahead of time will significantly reduce the size of, and churn generated by, this PR.

Yes it will conflict with this PR but it should be trivial to just run git checkout --thiers to accept the upstream changes since we know we want to basically revert all the conflicting parts of this PR, right?

(i.e. there should be no manual conflict resolution needed I think)

sbc100 · 2021-07-11T17:44:13Z

 int main() {
  // Create initial threads.
-  for(int i = 0; i < NUM_THREADS; ++i) {
+  for(long i = 0; i < NUM_THREADS; ++i) {


I think you can leave this as int, no?

I was just trying to make the code work with as few casts as possible. Since the thread id is stored as a long/pointer, it seemed nicer to make this loop use long instead of int.

sbc100

I would rather use intptr_t where is makes sense. I don't feel too strongly about this, but I would certainly like to see printf use %p for pointers where possible.

sbc100 · 2021-07-11T17:45:33Z

  assert(argc == 1);
  pc = (char *)alloca(4+argc);
-  assert(((int)pc) % 4 == 0);
+  assert(((long)pc) % 4 == 0);


Is this case needed? Can't it just be removed?

Was just trying to fix the existing test for 64-bit. It probably shouldn't be removed since this also still runs for 32-bit.

sbc100 · 2021-07-11T17:48:20Z

+long get_stack() { int i; return (long)&i; }
 int uses_stack(test_struct* t1) {
-  if (stackChecker == 0) stackChecker = (int*)malloc(sizeof(int));
+  if (stackChecker == 0) stackChecker = (long*)malloc(sizeof(long));


I can't see why we use malloc at all here.. why not just have static intptr_t stackChecker = 0 and avoid the malloc completely? (just store the value in memory)

I was not trying to fix the actual tests, just make this 64-bit compatible. I can fix this also if you want, but it would not make this PR a wasm32 NFC as intended.

sbc100 · 2021-07-11T17:56:39Z

 }

-volatile int fetch_and_or_data = 0;
+volatile long fetch_and_or_data = 0;


This seems like maybe it could be changing what we are testing here?

I was assuming there's equivalent 64-bit atomic ops for this, any reason why not?

aardappel · 2021-07-12T17:46:53Z

Browser CI fails on [0710/000005.004568:ERROR:socket_posix.cc(150)] bind() failed: Address already in use (98).. is that just flakey?

aardappel · 2021-07-12T18:33:09Z

Also an error in 3rd-party code (poppler?) unrelated to this PR it seems:

/root/cache/sysroot/include/c++/v1/__tree:2136:12: error: call to deleted constructor of 'std::__tree<int, std::less<int>, std::allocator<int> >::__node_holder' (aka 'unique_ptr<std::__tree_node<int, void *>, __tree_node_destructor<allocator<std::__tree_node<int, void *> > > >')
    return __h;
           ^~~
/root/cache/sysroot/include/c++/v1/__tree:2117:29: note: in instantiation of function template specialization 'std::__tree<int, std::less<int>, std::allocator<int> >::__construct_node<const int &>' requested here
        __node_holder __h = __construct_node(_VSTD::forward<_Args>(__args)...);
                            ^
/root/cache/sysroot/include/c++/v1/__tree:1255:16: note: in instantiation of function template specialization 'std::__tree<int, std::less<int>, std::allocator<int> >::__emplace_hint_unique_key_args<int, const int &>' requested here
        return __emplace_hint_unique_key_args(__p, _NodeTypes::__get_key(__v), __v).first;
               ^
/root/cache/sysroot/include/c++/v1/set:669:25: note: in instantiation of member function 'std::__tree<int, std::less<int>, std::allocator<int> >::__insert_unique' requested here
                __tree_.__insert_unique(__e, *__f);
                        ^
/root/cache/sysroot/include/c++/v1/set:534:13: note: in instantiation of function template specialization 'std::set<int>::insert<std::__tree_const_iterator<int, std::__tree_node<int, void *> *, long> >' requested here
            insert(__s.begin(), __s.end());
            ^
Form.cc:806:44: note: in instantiation of member function 'std::set<int>::set' requested here
            std::set<int> usedParentsAux = *usedParents;

aardappel · 2021-07-12T18:35:50Z

and a RuntimeError: unreachable in test_asyncify_indirect_lists which I didn't modify..

sbc100

We talking offline, and I'm good with doing a cleanup of our printf() usage as a followup.

There are a lot of places where pointers get cast to int, and to prepare for further Memory64 changes, those are changed to long. I chose long since it is most similar to the existing int, instead of e.g intptr_t, since long is more universal (there are many cases where the actual data stored is not a pointer). Since under wasm32 int and long are equal, this should in theory be a NFC for wasm32, making it easier to review. It also allows #12869 to be smaller

aardappel requested review from kripken and sbc100 July 9, 2021 20:55

kripken approved these changes Jul 9, 2021

View reviewed changes

sbc100 reviewed Jul 11, 2021

View reviewed changes

aardappel changed the title ~~[Memory64] Replace int with long in tests~~ [Memory64] Replace int (used as ptr) with long in tests Jul 12, 2021

sbc100 approved these changes Jul 12, 2021

View reviewed changes

aardappel enabled auto-merge (squash) July 15, 2021 22:43

aardappel merged commit f6b6ea0 into emscripten-core:main Jul 15, 2021

aardappel mentioned this pull request Sep 23, 2021

[wasm64] making JS bindings wasm64 aware #12869

Merged

Uh oh!

Conversation

aardappel commented Jul 9, 2021 • edited by sbc100 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 Jul 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 Jul 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aardappel commented Jul 12, 2021

Uh oh!

aardappel commented Jul 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aardappel commented Jul 12, 2021

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aardappel commented Jul 9, 2021 •

edited by sbc100

Loading

sbc100 Jul 11, 2021 •

edited

Loading

sbc100 Jul 11, 2021 •

edited

Loading

aardappel commented Jul 12, 2021 •

edited

Loading