These are chat archives for dropbox/pyston

Jul 2015
Chris Toshok
Jul 11 2015 01:06 UTC
do we have a good feeling of what’s going on with the .pyc file truncation/corruption?
Kevin Modzelewski
Jul 11 2015 03:19 UTC
not really
it seems to be writing out pyc files with the magic string for the wrong parser (pypa vs cpython)
@tjhance, did you look into the live-outs thing any more?
I'm running into it on my branch and I was going to code up the fix we talked about if you hadn't gotten to it
Travis Hance
Jul 11 2015 03:27 UTC
I hadn’t gotten to it
was planning to do it this weekend
oh I think I fixed the build on rewrite-starargs though
I need to follow up on all these old PRs though
Travis Hance
Jul 11 2015 03:41 UTC
i’ll just do it now
Chris Toshok
Jul 11 2015 04:15 UTC
hm, I’m seeing:
Warning: truncated .pyc file found; ignoring
/home/travis/build/dropbox/pyston/src/codegen/parser.cpp:1216: pyston::AST_Module *pyston::caching_parse_file(const char *): Assertion `tries <= MAX_TRIES' failed: repeatedly failing to parse file
which isn’t the same as the pypa/cpython magic string
oddly on the same test though
Chris Toshok
Jul 11 2015 04:26 UTC
(, that is)
Travis Hance
Jul 11 2015 04:29 UTC
@kmod maybe I am the wrong person for this… I don’t see how this patchpoint size calculation works
like this line
// 14 bytes per reg that needs to be spilled
call_size += 14 * 4;
why is it hardcoding 4 as the number of registers that need to be spilled
(in patchpoints.cpp)
is that callee-save or caller-save?
or… something else
oh I know
I’ll look at that diff you sent me
Travis Hance
Jul 11 2015 04:35 UTC
oh, the diff you sent me deals with the other call to registerCompiledPatchpoint (in ics.cpp)
Travis Hance
Jul 11 2015 04:41 UTC
right, that makes sense
because thisproblem is about ICs
Travis Hance
Jul 11 2015 06:58 UTC

@kmod I think I almost have this figured out… I finally realized I need to focus on initializePatchpoint3 since that’s where all the spilling and such is. So it seems like the live_outs fall into 3 camps: the callee save registers, the regs_to_spill, and the regs_to_reload.

So the first camp, callee save registers, is easy, since we basically don’t touch those.

The regs_to_reload are also easy. Since the continue_addr is now before these registers get unspilled, I can just remove them from the live_outs, as we discussed

I’m unsure about the regs_to_spill camp; it seems like they have to go in the live_outs (the “batch push” which spills them doesn’t run before the IC does, and even if it did, it uses the scratch space, the same space as the ICs)
which means we still need to add some restoreLiveOuts (analagous to restoreArgs, I guess) logic—which is what we were hoping to avoid, right?
Kevin Modzelewski
Jul 11 2015 09:46 UTC
oh, yeah the things that go into regs_to_spill would be problematic
either we need to make the rewriter do a better job of respecting them as live_outs, or we could just spill them around the entire patchpoint
maybe we could call spillFrameArgumentIfNecessary() on all the live outs as well
which would make regs_to_spill empty
err, well we'd want to do some refactoring in there, but the approach would be to get rid of the regs_to_spill case by having already spilled them
Travis Hance
Jul 11 2015 18:41 UTC
Hmm so I ended up just making the re writer restore the live outs (see the PR) although it seems like this just makes things more complicated and it probably produces worse assembly too
But anyway I came across something odd
I was trying to write a test case which would fail if we didn't restore the live outs st guards
I ended up finding a small python program where a setattr patchpoint takes RAX as a live out
But then the preserved value of RAX is never used
Which seems like it shouldnt happen
Travis Hance
Jul 11 2015 18:46 UTC
Cam you think of a reason that might happen? The live outs are provided by the llvm stack map, so I don't see how this could be our fault
The only possibility I can think of is that we'd need to preserve it for introspection or something, but I'm not sure how that works
I think llvm will tell us that rax is a live out but they really mean that the return will get used
not that the incoming value of rax will stay live
Travis Hance
Jul 11 2015 19:32 UTC
I don’t think so; for one thing, setattr doesn’t have a return value; second, it doesn’t use the resulting value of RAX at all
Travis Hance
Jul 11 2015 19:39 UTC
hm unfortunately i can’t check the stackmap documentation; seems down
Kevin Modzelewski
Jul 11 2015 23:43 UTC
hmm it doesn't seem like there should be any reason than rax is a live out then
the live-outs stuff doesn't seem to be very exact
and there have been quite a number of "it doesn't make sense why llvm says that that is a live out or not" cases over time
oh shoot nevermind, setattr uses the PreserveAll calling convention
I thought it was only getattr