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

Method for finding cold end of stack needs justification #210

Open
rptb1 opened this issue Apr 4, 2023 · 4 comments
Open

Method for finding cold end of stack needs justification #210

rptb1 opened this issue Apr 4, 2023 · 4 comments
Labels
documentation needs analaysis The issue needs analysis before it can be resolved. optional Will cause failures / of benefit. Worth assigning resources.

Comments

@rptb1
Copy link
Member

rptb1 commented Apr 4, 2023

The manual advises the use of a stack marker to find the cold end of the stack:

void *marker = ▮
mps_root_t stack_root;
res = mps_root_create_thread(&stack_root, arena, thread, marker);
if (res != MPS_RES_OK) error("Couldn't create stack root");
In order to scan the control stack, the MPS needs to know where the
:term:`cold end` of the stack is, and that's the role of the
:c:data:`marker` variable: the compiler places it on the stack, so its
address is a position within the stack. As long as you don't exit from
this function while the MPS is running, your program's active local
variables will always be placed on the stack after :c:data:`marker`,
and so will be scanned for references by the MPS.

Is this advice reliable with current compiler optimisations on our target platforms?

We should check and record our justification for it. We should also test for it, for example, by asserting about it in our tests, so that we are warned if it ever becomes false.

Incidentally, on this topic, design.mps.stack-scan.req.stack.cold.not says:

_`.req.stack.cold.not`: There is no requirement to locate the cold end
of the stack. (The mutator supplies this as an argument to
``mps_root_create_thread()``.)

@rptb1 rptb1 added needs analaysis The issue needs analysis before it can be resolved. documentation labels Apr 11, 2023
@thejayps thejayps added the optional Will cause failures / of benefit. Worth assigning resources. label Apr 11, 2023
@rptb1
Copy link
Member Author

rptb1 commented Apr 13, 2023

We now have proof that the statement from the manual is false. A cold-end marker obtained this way does not necessarily include roots in the same stack frame. So the statement that the program's active local variables are placed after the marker is false. To get a reliable placement warmer than the marker you need to be in a child stack frame.

JPH proved this while editing the arena extension/contraction test.

thejayps added a commit that referenced this issue Apr 14, 2023
This avoids the issue #210

Also increase the number of test objects by *10 to make it more likely
the arena will decide to contract.

Also comment out some printfs in the interrupt context, to avoid messy
output.

Also fix some typos in comments
rptb1 added a commit that referenced this issue Apr 14, 2023
@rptb1
Copy link
Member Author

rptb1 commented Apr 14, 2023

Even with b4d5485 in place, extcon.c fails with gcc 11.3.0 under Ubuntu 22, apparently because gcc inlines test_main into main and reorders the locals. 24e4c86 contains a temporary workaround.

rptb1 added a commit that referenced this issue Apr 14, 2023
… an reordering locals, causing stack roots to become lost. Working around GitHub issue #210 <#210>.
@rptb1
Copy link
Member Author

rptb1 commented Apr 14, 2023

627ded3 contains a further workaround that was necessary on clang 14.

rptb1 added a commit that referenced this issue May 2, 2023
…old end of the stack for now, deferring solution of GitHub Issue #210 <#210>.  Convertin asserts to Insists so that they are present in hot builds.
@thejayps
Copy link
Contributor

addrobj.c has also been written in #223 subsequent to the creation of this issue in a way which avoids the issue (i.e. registering a static region of memory to hold ambiguous pointers rather than register the stack and store them there.) It's not known whether the issue would exist in addrobj.c had it been written in a way which used stack roots.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation needs analaysis The issue needs analysis before it can be resolved. optional Will cause failures / of benefit. Worth assigning resources.
Projects
None yet
Development

No branches or pull requests

2 participants