[Toybox] Graff and bc

[Gavin Howard]

>> Before I answer your questions, I have good news: in the midst of
>> fixing bugs, I was still able to reduce the line count, even though I
>> had to add quite a bit to fix bugs.
>
> Winding a spring tighter and figuring out how to make a watch with half as much
> metal are two different approaches, and you often have to undo one to work out
> the other, but let me know when you've come to a stop and I'll look at it again...

I miscommunicated. Sorry about that. What I meant is that I was both
reducing lines of code and fixing bugs, but separately. And even
though I was fixing bugs, which was adding lines of code, I managed to
reduce enough that it was a net reduction.

However, in this case, fixing one bug is actually what helped reduce
lines of code. Once the bug was fixed, the duplication was clearer.

More about stopping later.

>>>> I am about to try to implement your changes to bc_exec (and maybe
>>>> bc_vm_init) in my release script. While doing so, if you would like,
>>>> it would make sense to try to remove the BcStatus enum and replace it
>>>> with either #defines (chars, so I can assign right to toys.retval) or
>>>> something else of your choice. What would you like me to do?
>>>
>>> Do we ever care about a bc command return status other than "nonzero"? Because
>>> if so, you might as well just have error strings?
>>
>> Technically, POSIX has no requirements here, other than returning
>> non-zero, but more on this later.
>
> That's why it was a question.

I was vague here.

I am going to fight tooth and nail to make sure the POSIX spec is
followed, but I do not care so much on things that are not in the
spec. Thus, the error messages, which are required in POSIX, are
important to me, as I tried to make clear in the last email, but error
codes, which are not required in POSIX (other than zero and non-zero),
are not so important.

However, here is my opinion: I think the error codes are useful
because bc can also be used by shell scripts. Plus, having a lookup
table saves a lot of code, even if it increases read-only data in the
executable.

>>> Let's see...
>>>
>>> grep -o 'BC_STATUS_[_A-Z]*' toys/*/bc.c | sort | uniq -c | sort -n
>>>
>>> 6 of them have an occurrence count of 1, which means they're never used (in the
>>> enum, never referred to elsewhere.)
>>
>> Some of those were supposed to be used. I fixed that. One was useless
>> for toybox. I changed the release script to remove it.
>
> I saw your emails submitting the code to busybox. (Well, as of a few days ago, I
> generally look at that folder weekly-ish.) They'll probably accept whatever you
> give them verbatim, so I doubt you'll get much pushback from that direction.

I'm not sure what you were trying to tell me here. I meant to report
back on what I did with those error codes you had identified. I
removed the one you did not need. The others were still needed,
unfortunately. Thanks for letting me know that I was not doing as
thorough error checking as I needed to.

>>> 32 of them have an occurrence count of 2: used exactly once.
>>>
>>> 5 occur 3 times, 4 occur 4 times, 5 occur 5 times, and so on up. So several
>>> occur repeatedly, and I'd have to look at why...
>>
>> The biggest reason there are so many is because I want better error
>> messages than the GNU bc, which can be cryptic.
>
> Better error messages are good.
>
> Is there a tutorial on how to use bc anywhere?

I'm not sure. It would be useful to make one. A bc with GNU extensions
is actually fairly useful.

>> The bc spec defines a lot of places where errors could occur, but then
>> it says that only two or three errors are necessary. The GNU bc seems
>> to have taken it at its word, but I put in an error for every single
>> one, in an attempt to make it easier on users to write and debug
>> scripts, as well as use the bc as a REPL.
>
> And naturally toybox and busybox are the logical places for that.

I think you were trying to tell me that you do not like using toybox
and busybox as the places to put bc for writing and debugging bc
scripts. Fair point. But the only bc you can use right now is the one
I wrote. Unfortunately, I am human too, with human preferences. That's
what I wanted when I wrote it.

And as I said, I don't intend for this to just go into toybox.

>>> I'm still not sure the difference between LEX_EOF and PARSE_EOF, one is used 17
>>> times and the other 11 but you're converting one into the other in some places
>>> here...
>>
>> I have already removed PARSE_EOF in my PR. It was legacy.
>>
>>> Of course STATUS_SUCCESS is used 91 times. MALLOC_FAIL is 19 (see virtual vs
>>> physical memory rant)...
>>>
>>> tl;dr: I need more context and code review before forming a strong opinion about
>>> the right thing to do here, and if I start I'll just start chipping away at
>>> low-hanging fruit again. Waiting for you to come to a good stopping point first...
>>>
>>> But I _suspect_ you can probably deal with the errors without having a lookup
>>> table of signals between error producer and error consumer that's your own code
>>> on both sides. That's the kind of thing that becomes clear once enough
>>> overgrowth is cleaned up that the pieces get closer together.
>>
>> You may be right, but I do not think so. Yet.
>
> Oh there's work to do to get there, sure.

Okay.

>> I chose to use a table lookup because of the requirement to print
>> error messages. The reason was that I wanted only one place where
>> errors were handled, or at least, few places. That is why I wrote the
>> print wrappers for error messages. And there was another reason: bc
>> has to choose to exit on error if not interactive, or to just display
>> the error message.
>
> I haven't reviewed far enough into the code to see what I'd do.
>
>> If I had not done that, I would have had to write something like:
>>
>> if (error) bc_error("Some error message")
>>
>> And in bc_error,
>>
>> void bc_error(char *msg) {
>>   printf(msg);
>>   if (!bc_interactive) exit(1)
>> }
>>
>> I mean, that's more than feasible, I admit it. And it would virtually
>> eliminate the 202 instances of
>>
>> if (status) return status;
>
> The toybox _exit() stuff has the ability to longjump() rather than exit for a
> reason. But then you need to register your resources to they can be cleaned up
> centrally. (I have vague plans to look at that in toysh, but it's down the road
> a way.)
>
> Again, I don't have test code for bc, have never used it interactively for
> nontrivial things, and haven't seen a tutorial on it. It's on the todo list.
>
>>  But it would sprinkle error messages throughout the code. Putting
>> them all in one place, corresponding to a specific labeled error is
>> clearer in the source code, at least to me.
>>
>> The other reason I used a lookup table with a error producers and an
>> error consumer (BcVm) was so that I could clean up properly on exit.
>> No you don't want to do that (see above, putting bc_vm_free() under
>> CFG_TOYBOX_FREE), but there are cases when it *is* necessary, as
>> evidenced by the very existence of CFG_TOYBOX_FREE.
>
> Cleaning up code is a bit like unraveling knots. You see duplicated stuff, and
> see if you can move it around and get it to line up so it can collapse together.
> I'm just noting duplicate stuff.
>
>> Of course, I could still clean up on exit with a bc_error() like above
>> if I could make the one instance of BcVm global, but I don't want to
>> do that, and I don't think you would want me to do it either. If I am
>> wrong, let me know, along with your reasons. I am open to changing it
>> for the right reasons. One of those might be the fact that you want to
>> eliminate
>
> I haven't even started working out the object lifetime rules yet. I dunno what
> you're allocating, in what order, how long it lasts before it's freed, and what
> happens in between. For all I know there could be a simple "gimme_vector()"
> function that adds it to a linked list and then a "scrap_vectors()" that frees
> everything after a checkpoint we just longjmped back to. Or maybe it's multiple
> stacks. Or maybe it's a tree a clean() function could run against. I don't know
> yet. I touched it one evening, and then you went away and did over a dozen
> commits since.
>
>> if (status) return status;
>>
>> instances. However, even in that case, I think that I would like to
>> still use a lookup table, to avoid sprinkling them throughout the
>> code.
>>
>> But who knows?
>
> Not me. Let me know when it stops moving again.

After reading this email and asking people who are better than me at
working with people, I realized that the big theme was you asking me
to just stop and give the bc to you. I think it happened three times.

Once I realized that, I realized the big problem we have: there was a
key miscommunication. You wanted me to write the bc and just hand it
off to you. I thought you wanted a minimum workable prototype fast. (I
think you said something along the lines of "let me know when it can
run the timeconst.bc script.") Of course, I could be wrong about what
you wanted; correct me if I am.

However, if it is the case, you will have to wait a while for me to
get to a stopping point. I do not like to hand off software at all (I
will always feel *some* obligation to maintain it, probably a
psychological quirk), but if I have to, I will only do it once I am
six nines percent sure that the software contains no critical bugs,
especially security vulnerabilities.  Up until that point, I will not
have a stopping point; I will be continually making changes, which
include both fixing bugs and reducing the lines of code, so I cannot,
in good faith, give it to you because you do seem eager to have it
(you started looking at it right away). I was planning on it being in
pending for awhile while I made it bulletproof, but after you started
on it, I realized that you were probably intent on putting it in posix
as soon as possible. However, that means that it needs to be
completely ready before you get it, so you can't just get a minimum
viable product; you need a *bulletproof* product.

All of that preparation is made even more essential by the fact that
you seem to want to take it and make a lot of changes, whether or not
you want me maintain it. The more I can satisfy your desires for it
(reduce loc is the big one), the less you will probably change and the
more likely the two versions will remain in sync.

So with all that said, here is what I am going to accomplish before I
get to a stopping point:

1) It needs a comprehensive test suite, and it needs to pass it all.
All the time.
2) It needs to go through two full release cycles into the wild
without showstopper (any essential functionality broken) or security
bugs.
3) It needs to be thoroughly fuzzed.

And those are just my minimum requirements.

I know that doing those things will take awhile (at least six months
if both initial releases don't dig up showstopper bugs). But it will
prevent you from having to debug something the like the recursion bug
I squashed a few days ago. It was not fun, and I only noticed it by
luck and by knowing the code like the back of my hand, both of which
you cannot count on.

So for now, I will close my PR and continue working on the bc to
harden it. I ask for your patience.

Gavin Howard