Replace C NULL for iteration stop with StopIteration #2023
Conversation
Instead of returning a C NULL when we want to stop iteration, we now raise the StopIteration exception. This is a prerequisite for doing unboxed values since we need the NULL value for unboxed values. Longer term, we might go back to using NULL but packed in a tuple, like a tagged union basically, which is likely faster than raising an exception (that uses longjmp) but that requires some other support.
|
@sydow I have found a bug which is reproducible just in the Acton repository. I'm not sure if this is because of the same issue as in the respnet repo or not, but it's at least a difference in behavior that seems to come with this PR! commands to reproduce: output: The messages |
|
I've located the bug, it's the CPS pass that contains a leak. Fix is coming up. |
|
I don’t want to compete about being the source of the bug, but I also found a bug that may be involved. This bug is easy to fix and I did so locally, but it does not repair the hello example, so maybe both bugs are involved? I have not yet pushed this, waiting for comments about further things I may need to do (see below).
Here is an example where my bug is involved:
```
def f(res):
try:
res = 1
raise ValueError()
except ValueError:
pass
return res # should return 1
actor main(env):
print(f(0))
env.exit(0)
```
The interesting variable res is now a parameter to f. The above code erroneously prints 0, since the parameter is not declared volatile by the naive algorithm I used in my fix the other day. This algorithm only looks at the statement sequence in which the try/except statements occur, scanning it backwards from end to start looking for variable declarations where we need a volatile annotation. We need to continue to do this to the parameters of the fun (or actor) declaration of which the statement sequence is the body. I need to think about other cases (state vars in classes?).
The reason I think that this bug may affect the hello example is the following fragment from cli/acton.act:
```
def warn_on_large_zig_global_cache(cap: file.FileCap):
< beginning of code omitted for space reasons>
total_size: int = 0
for f in fs.walk(cache_dir):
print ("f=",f,"size=",f.size)
total_size += int(f.size)
gb = 1024 * 1024 * 1024
if total_size > warn_level * gb:
<code for this case also omitted>
if total_size == 0:
print("INFO: Acton global cache is empty: rebuilding common libraries (e.g. libc) which will take some time...")
```
Here it is obvious that total_size must be declared volatile, which it would be using my naive algorithm, but CPS conversion enters and makes total_size be involved as a parameter to (several) C procedures and as a field in C structures, so I get lost in the code and cannot really see what the fix may be in the C code, even less how to achieve this in CodeGen.hs. But then maybe something went wrong in CPS conversion...
|
|
I believe we have identified two separate bugs! The extended search for volatile variables looks obvious once it's explained, although it hadn't occurred to me... But the example of building The core obstacle is that our language doesn't allow non-local reassignment, so we can't even represent the outcome of certain CPS-conversions in a straightforward way (and the lambda-lifter is consequently not prepared to handle them). As a remedy, we add reassigned variables as extra parameters to the generated continuations already in the CPS pass, as a kind of "specialized pre-mature lambda-lifting". It's just that this trick doesn't blend well with the irregular transfer of control of CPS'ed exception handlers. In fact, I suspect this might be exactly the same problem that renders regular register allocation in the C compiler unsuitable for variables that are reassigned right before a Need to ponder how to resolve this. Perhaps the non-local reassignments we've talked about introducing when we generalize the In any case, fixing the CPS analysis so that it doesn't convert more exception handlers than strictly necessary might fix the present issue. But we mustn't fool ourselves into believeing this causes the core problem to go away. |
|
And fixing the overly eager CPS converter immediately exposes the problem Björn described. Intricate! |
|
I've pushed my small fix that avoid unneeded CPS transation. It doesn't resolve the issue by itself, but will most likely do so in combination with Björn's pending commit. But the fundamental problem with CPS/lambda-lifting of exception handlers remains unsolved for now :-( |
|
And I have pushed my even smaller fix which extends volatile annotations to function parameters. And indeed, now the hello example works. But more thinking is needed…
Björn
… On 17 Dec 2024, at 15:06, Johan Nordlander ***@***.***> wrote:
I've pushed my small fix that avoid unneeded CPS transation. It doesn't resolve the issue by itself, but will most likely do so in combination with Björn's pending commit. But the fundamental problem with CPS/lambda-lifting of exception handlers remains unsolved for now :-(
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
@sydow @nordlander FYI, it is still broken. CI testing shows almost all test are passing, but it's actually not. If we look in test-macos or test-linux, we can see that To reproduce, run |
|
I'd really like to have some test cases for the various failing cases that we are seeing now, so we don't just rely on whatever the acton cli happens to look like right now |
|
Okay, so I added a new test program that includes @sydow example from above as well as what I believe is the failing patter we saw in the Feel free to rename the test program or whatnot, I don't know what this is called. |
|
Whops, I just noticed I wasn't using the latest version of this branch. The But okay, so there's got to be some other interesting interaction of features going on... |
|
I have rebased this work on the latest main, pushed to a new branch and opened PR #2054 for the new branch, which supersedes this one and thus I'm closing this PR. |
No description provided.