The trouble with rewriting 'unused' to '__attribute__(unused)' is that
if we happen to deliberately introduce '__attribute__(unused)' somehow,
say in the standard headers, then it gets expanded twice to '__attribute__(__attribute__(unused))'.
So we switch to a synonym.
Clean up how we reclaim local scopes.
It used to work like this (commit 3216):
1. Update refcounts of products after every instruction, EXCEPT:
a) when instruction is a non-primitive and the callee starts with
'local-scope' (because it's already not decremented in 'return')
OR:
b) when instruction is primitive 'next-ingredient' or
'next-ingredient-without-typechecking', and its result is saved to a
variable in the default space (because it's already incremented at
the time of the call)
2. If a function starts with 'local-scope', force it to be reclaimed
before each return. However, since locals may be returned, *very
carefully* don't reclaim those. (See the logic in the old `escaping`
and `should_update_refcount` functions.)
However, this approach had issues. We needed two separate commands for
'local-scope' (reclaim locals on exit) and 'new-default-space'
(programmer takes charge of reclaiming locals). The hard-coded
reclamation duplicated refcounting logic. In addition to adding
complexity, this implementation failed to work if a function overwrites
default-space after setting up a local-scope (the old default-space is
leaked). It also fails in the presence of continuations. Calling a
continuation more than once was guaranteed to corrupt memory (commit
3986).
After this commit, reclaiming local scopes now works like this:
Update refcounts of products for every PRIMITIVE instruction.
For non-primitive instructions, all the work happens in the `return`
instruction:
increment refcount of ingredients to `return`
(unless -- one last bit of ugliness -- they aren't saved in the
caller)
decrement the refcount of the default-space
use existing infrastructure for reclaiming as necessary
if reclaiming default-space, first decrement refcount of each
local
again, use existing infrastructure for reclaiming as necessary
This commit (finally!) completes the bulk[1] of step 2 of the plan in
commit 3991. It was very hard until I gave up trying to tweak the
existing implementation and just test-drove layer 43 from scratch.
[1] There's still potential for memory corruption if we abuse
`default-space`. I should probably try to add warnings about that at
some point (todo in layer 45).
Standardize exit paths. Most layers now don't need to know about termbox.
We can't really use `assert` in console-mode apps; it can't just exit because
we want to be able to check assertion failures in tests.
This was a large commit, and most of it is a follow-up to commit 3309,
undoing what is probably the final ill-considered optimization I added
to s-expressions in Mu: I was always representing (a b c) as (a b . c),
etc. That is now gone.
Why did I need to take it out? The key problem was the error silently
ignored in layer 30. That was causing size_of("(type)") to silently
return garbage rather than loudly complain (assuming 'type' was a simple
type).
But to take it out I had to modify types_strictly_match (layer 21) to
actually strictly match and not just do a prefix match.
In the process of removing the prefix match, I had to make extracting
recipe types from recipe headers more robust. So far it only matched the
first element of each ingredient's type; these matched:
(recipe address:number -> address:number)
(recipe address -> address)
I didn't notice because the dotted notation optimization was actually
representing this as:
(recipe address:number -> address number)
---
One final little thing in this commit: I added an alias for 'assert'
called 'assert_for_now', to indicate that I'm not sure something's
really an invariant, that it might be triggered by (invalid) user
programs, and so require more thought on error handling down the road.
But this may well be an ill-posed distinction. It may be overwhelmingly
uneconomic to continually distinguish between model invariants and error
states for input. I'm starting to grow sympathetic to Google Analytics's
recent approach of just banning assertions altogether. We'll see..
I had this early on. Why did I delete it? Was it because I was only
flushing the current trace when I started on the next one? We don't have
that problem anymore..
Fix CI.
Figuring out this memory leak was an epic story. I was able to quickly
track down that it was caused by commit 3365, in particular the
overloading of to-text to support characters. But beyond that I was
stumped. Why were layer 3's trace_stream::curr_stream objects being
leaked in layer 81 by a change in layer 59?!
Triaging through the layers, I found the following:
layer 81 - leaks 2 blocks (in clear-line-erases-printed-characters)
layer 83 - leaks 4 additional blocks (in clear-line-erases-printed-characters-2)
I also figured out that the leaks were happening because of the call to
'trace' on a character inside print:character (that's the 'print'
function called with a character argument)
trace 90, [print-character], c
So I tried to create a simple scenario:
scenario trace-on-character-leaks [
1:character <- copy 111/o
trace 90, [print-character], 1:character
]
But that triggered no leaks. Which made sense because there were plenty
of calls to that 'trace' instruction in print:character. The leak only
happened when print:character was called from clear-line. Oh, it happens
only when tracing 0/nul characters. Tracing a Mu string with a nul
character creates an empty C++ string, which is weird. But why should it
leak memory?!
Anyway, I tried a new scenario at layer 62 (when 'trace' starts
auto-converting characters to text)
scenario stashing-nul-character-leaks [
1:character <- copy 0/nul
trace 90, [dbg], 1:character
]
But still, no leak! I played around with running layers until 70, 80.
But then it didn't leak even at layer 82 where I'd seen it leak before.
What had I done?
Turns out it was only leaking if I used names for variables and not
numeric addresses. Eventually I was able to get layer 59 to leak:
scenario stashing-nul-character-leaks [
c:character <- copy 0/nul
x:text <- to-text c
trace 90, [dbg], x
]
At that point I finally went to look at layer 3 (I'd been thinking about
it before, but hadn't bothered to *actually go look*!) And the leak was
obvious.
In the end, all the information I needed was right there in the leak
report. The reason this was hard to find was that I wasn't ready to
believe there could be a bug in layer 3 after all these months. I had to
go through the five stages of grief before I was ready for that
realization.
Final mystery: why was the memory leak not triggered by numeric
variables? Because the transform to auto-convert ingredients to text
only operated on named variables. Manually performing the transform did
leak:
scenario stashing-text-containing-nul-character-leaks [
1:text <- new character:type, 1/capacity
put-index *1:text, 0, 0/nul
trace 90, [dbg], 1:text
]
Undo 3272. The trouble with creating a new section for constants is that
there's no good place to order it since constants can be initialized
using globals as well as vice versa. And I don't want to add constraints
disallowing either side.
Instead, a new plan: always declare constants in the Globals section
using 'extern const' rather than just 'const', since otherwise constants
implicitly have internal linkage (http://stackoverflow.com/questions/14894698/why-does-extern-const-int-n-not-work-as-expected)
Move global constants into their own section since we seem to be having
trouble linking in 'extern const' variables when manually cleaving mu.cc
into separate compilation units.
Clean up the Globals section so that we can generate extern declarations
for all globals out using this command after we carve it out into
globals.cc:
grep ';' globals.cc |perl -pwe 's/[=(].*/;/' |perl -pwe 's/^[^\/# ]/extern $&/' > globals.h
The first perl command strips out initializers. The second prepends
'extern'. This simplistic approach requires each global definition to
lie all on one line.
Deconstruct the tracing layer which had been an exception to our
includes-types-prototypes-globals-functions organization thus far.
To do this we predefine a few primitive globals before the types that
use them, and we pull some method definitions out of struct definitions
at the cost of having to manually write a couple of prototypes.
I started out incredibly lax about running past errors (I even used to
call them 'warnings' when I started Mu), but I've been gradually seeing
the wisdom of Go and Racket in refusing to run code if it doesn't pass
basic integrity checks (such as using a literal as an address).
Go is right to have no warnings, only errors. But where Go goes wrong is
in even caring about unused variables.
Racket and other languages perform more aggressive integrity checks so
that the can optimize more aggressively, and I'm starting to realize I
don't know enough to disagree with them.
When updating refcounts for a typed segment of memory being copied over
with another, we were only ever using the new copy's data to determine
any tags for exclusive containers. Looks like the right way to do
refcounts is to increment and decrement separately.
Hopefully this is a complete fix for the intermittent but
non-deterministic errors we've been encountering while running the edit/
app.
I'd been toying with this idea for some time now given how large the
repo had been growing. The final straw was noticing that people cloning
the repo were having to wait *5 minutes*! That's not good, particularly
for a project with 'tiny' in its description. After purging .traces/
clone time drops to 7 seconds in my tests.
Major issue: some commits refer to .traces/ but don't really change
anything there. That could get confusing :/
Minor issues:
a) I've linked inside commits on GitHub like a half-dozen times online
or over email. Those links are now liable to eventually break. (I seem
to recall GitHub keeps them around as long as they get used at least
once every 60 days, or something like that.)
b) Numbering of commits is messed up because some commits only had
changes to the .traces/ sub-directory.
This is hopefully quite thorough. I handle nested containers, as well as
exclusive containers that might contain addresses only when the tag has
a specific value.
Since we switched to end() for terminating trace lines, there's a lot
less reason to avoid this. We don't nest trace statements either
anymore.
I'd like to not hide warnings and still be able to make assertions on
their absence so that printed warnings also express as failed tests.
Now we're back to trying to rerunning idempotent transforms on
specialized recipes. Still doesn't work, but at least we don't see
different results depending on whether the trace is enabled inside the
test or right at the start. That got fixed by the more disciplined
insertion into maps, looks like.
There were several places where we push a call on to a routine without
incrementing call-stack depth, which was used to compute the depth at
which to trace an instruction. So sometimes you ended up one depth lower
than you started a call with. Do this enough times and instructions that
should be traced at level 100 end up at level 0 and pop up as errors.
Solution: since call-stack depth is only used for tracing, include it in
the trace stream and make sure we reset it along with the trace stream.
Then catch all places where we forget to increment call-stack depth and
make sure we catch such places in the future.
When I first ran into this with Caleb I thought there must be some way
that we're writing some output into the warnings result. I didn't
recognize that the spurious output as part of the trace, just at the
wrong level.
At the lowest level I'm reluctantly starting to see the need for errors
that stop the program in its tracks. Only way to avoid memory corruption
and security issues. But beyond that core I still want to be as lenient
as possible at higher levels of abstraction.
Kartik Agaram