From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id EB600C11F64 for ; Thu, 1 Jul 2021 20:22:10 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id CFA3461413 for ; Thu, 1 Jul 2021 20:22:09 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CFA3461413 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=konsulko.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 17DFE82BE7; Thu, 1 Jul 2021 22:22:08 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=konsulko.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=konsulko.com header.i=@konsulko.com header.b="CLLQg3h3"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id C6E9B82C29; Thu, 1 Jul 2021 22:22:05 +0200 (CEST) Received: from mail-qk1-x72b.google.com (mail-qk1-x72b.google.com [IPv6:2607:f8b0:4864:20::72b]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 2207B82BDD for ; Thu, 1 Jul 2021 22:22:01 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=konsulko.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=trini@konsulko.com Received: by mail-qk1-x72b.google.com with SMTP id f6so7360246qka.0 for ; Thu, 01 Jul 2021 13:22:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=konsulko.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=OlsGtYvVzC36lLzaeoA73yJfCF3idW6AWfM1AeKSSsI=; b=CLLQg3h3FHZ49R1fp5PiTgwfvNbyayGj70LXaCXp+PhX6My7P5N0BGkloR2QZJa3Yj FuQuHYUwKOVbAP+Wqd3MUTKmiLPpow563GLkJByNinRlBsDIVbUzVG9MK4H6xhWUqK6n AyT/u36/amZ21vVg9y+n8Q425qyQ/0H1Mi7UA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=OlsGtYvVzC36lLzaeoA73yJfCF3idW6AWfM1AeKSSsI=; b=I+4X+CanNbRz9qZCv2+/RkYr41jKmbGOXfHlb1OID5xq+CKHmZI0ZTJi8NLLiYSHCz 73/izm1yYBc049e/IGFVFy6P2IgFOSsLNPEOT4kkdD2qjXTZdoYq153Ph1hrW3Q4WA83 9VvWwXnwJknfDL0unIMHRjse5JNRiWKnCr9g2L9575ad3UYXoqAwP9Ywewm+nRn7g1Lq y8S6hDx1n886k+DoqoXNFQ8Nqfx/Ewf7qMTLQp1Rjtxd4iYo5FFAB5AO/yejdVQdulg/ E3wB9NV0JeL96oqFgqtu5pVA7ngauOEfGjExhD6+OAd9awOUyPrWRCbM4ehT/CxeLuuw xttQ== X-Gm-Message-State: AOAM532U+tv6VHWM1VBrCTGfkHylFqDdpLv3IoE9xRpo4BpE/LMJ1HMU 26dJxt1fBeAG84sSvomtLXtmRw== X-Google-Smtp-Source: ABdhPJyb8AsuXsxcrpQEQCgZUssIUvbFygZwooJHIqqoijuYT91LgfbxnAJz9+Z9JhE2gxEr2VgTUA== X-Received: by 2002:a05:620a:a53:: with SMTP id j19mr1765845qka.482.1625170919829; Thu, 01 Jul 2021 13:21:59 -0700 (PDT) Received: from bill-the-cat (2603-6081-7b01-cbda-91af-d604-26f4-428b.res6.spectrum.com. [2603:6081:7b01:cbda:91af:d604:26f4:428b]) by smtp.gmail.com with ESMTPSA id c11sm394238qth.29.2021.07.01.13.21.56 (version=TLS1_2 cipher=ECDHE-ECDSA-CHACHA20-POLY1305 bits=256/256); Thu, 01 Jul 2021 13:21:57 -0700 (PDT) Date: Thu, 1 Jul 2021 16:21:55 -0400 From: Tom Rini To: Sean Anderson Cc: u-boot@lists.denx.de, Marek =?iso-8859-1?Q?Beh=FAn?= , Wolfgang Denk , Simon Glass , Roland Gaudig , Heinrich Schuchardt , Kostas Michalopoulos Subject: Re: [RFC PATCH 00/28] cli: Add a new shell Message-ID: <20210701202155.GQ9516@bill-the-cat> References: <20210701061611.957918-1-seanga2@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="J6AVObV96GkhyIUE" Content-Disposition: inline In-Reply-To: <20210701061611.957918-1-seanga2@gmail.com> X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.9.4 (2018-02-28) X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean --J6AVObV96GkhyIUE Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jul 01, 2021 at 02:15:43AM -0400, Sean Anderson wrote: > Well, this has been sitting on my hard drive for too long without feedback > ("Release early, release often"), so here's the first RFC. This is not re= ady to > merge (see the "Future work" section below), but the shell is functional = and at > least partially tested. >=20 > The goal is to have 0 bytes gained over Hush. Currently we are around 800= bytes > over on sandbox. A good goal, but perhaps slightly too strict? >=20 > add/remove: 90/54 grow/shrink: 3/7 up/down: 12834/-12042 (792) >=20 > =3D Getting started >=20 > Enable CONFIG_LIL. If you would like to run tests, enable CONFIG_LIL_FULL= =2E Note > that dm_test_acpi_cmd_dump and setexpr_test_str_oper will fail. CONFIG_LI= L_POOLS > is currently broken (with what appears to be a double free). >=20 > For an overview of the language as a whole, refer to the original readme = [1]. >=20 > [1] http://runtimeterror.com/tech/lil/readme.txt >=20 > =3D=3D Key patches >=20 > The following patches are particularly significant for reviewing and > understanding this series: >=20 > cli: Add LIL shell > This contains the LIL shell as originally written by Kostas with some > major deletions and some minor additions. > cli: lil: Wire up LIL to the rest of U-Boot > This allows you to use LIL as a shell just like Hush. > cli: lil: Document structures > This adds documentation for the major structures of LIL. It is a good > place to start looking at the internals. > test: Add tests for LIL > This adds some basic integration tests and provides some examples of > LIL code. > cli: lil: Add a distinct parsing step > This adds a parser separate from the interpreter. This patch is the > largest original work in this series. > cli: lil: Load procs from the environment > This allows procedures to be saved and loaded like variables. >=20 > =3D A new shell >=20 > This series adds a new shell for U-Boot. The aim is to eventually replace= Hush > as the primary shell for all boards which currently use it. Hush should be > replaced because it has several major problems: >=20 > - It has not had a major update in two decades, resulting in duplication = of > effort in finding bugs. Regarding a bug in variable setting, Wolfgang r= emarks >=20 > So the specific problem has (long) been fixed in upstream, and > instead of adding a patch to our old version, thus cementing the > broken behaviour, we should upgrade hush to recent upstream code. >=20 > -- Wolfgang Denk [2] >=20 > These lack of updates are further compounded by a significant amount of > ifdef-ing in the Hush code. This makes the shell hard to read and debug. > Further, the original purpose of such ifdef-ing (upgrading to a newer H= ush) > has never happened. >=20 > - It was designed for a preempting OS which supports pipes and processes.= This > fundamentally does not match the computing model of U-Boot where there = is > exactly one thread (and every other CPU is spinning or sleeping). Worki= ng > around these design differences is a significant cause of the aformenti= oned > ifdef-ing. >=20 > - It lacks many major features expected of even the most basic shells, su= ch > as functions and command substitution ($() syntax). This makes it diffi= cult > to script with Hush. While it is desirable to write some code in C, muc= h code > *must* be written in C because there is no way to express the logic in = Hush. >=20 > I believe that U-Boot should have a shell which is more featureful, has c= leaner > code, and which is the same size as Hush (or less). The ergonomic advanta= ges > afforded by a new shell will make U-Boot easier to use and customize. >=20 > [2] https://lore.kernel.org/u-boot/872080.1614764732@gemini.denx.de/ First, great! Thanks for doing this. A new shell really is the only viable path forward here, and I appreciate you taking the time to evaluate several and implement one. > =3D Open questions >=20 > While the primary purpose of this series is of course to get feedback on = the > code I have already written, there are several decisions where I am not s= ure > what the best course of action is. >=20 > - What should be done about 'expr'? The 'expr' command is a significant p= ortion > of the final code size. It cannot be removed outright, because it is us= ed by > several builtin functions like 'if', 'while', 'for', etc. The way I see= it, > there are two general approaches to take >=20 > - Rewrite expr to parse expressions and then evaluate them. The parsing= could > re-use several of the existing parse functions like how parse_list do= es. > This could reduce code, as instead of many functions each with their = own > while/switch statements, we could have two while/switch statements (o= ne to > parse, and one to evaluate). However, this may end up increasing code= size > (such as when the main language had evaluation split from parsing). >=20 > - Don't parse infix expressions, and just make arithmetic operators nor= mal > functions. This would affect ergonomics a bit. For example, instead of >=20 > if {$i < 10} { ... } >=20 > one would need to write >=20 > if {< $i 10} { ... } >=20 > and instead of >=20 > if {$some_bool} { ... } >=20 > one would need to write >=20 > if {quote $some_bool} { ... } >=20 > Though, given how much setexpr is used (not much), this may not be su= ch a > big price to pay. This route is almost certain to reduce code size. So, this is a question because we have cmd/setexpr.c that provides "expr" today? Or because this is a likely place to reclaim some of that 800 byte growth? > - How should LIL functions integrate with the rest of U-Boot? At the mome= nt, lil > functions and procedures exist in a completely separate world from norm= al > commands. I would like to integrate them more closely, but I am not sur= e the > best way to go about this. At the very minimum, each LIL builtin functi= on > needs to get its hands on the LIL interpreter somehow. I'd rather this = didn't > happen through gd_t or similar so that it is easier to unit test. > Additionally, LIL functions expect an array of lil_values instead of st= rings. > We could strip them out, but I worry that might start to impact perform= ance > (from all the copying). I might be missing something here. But, given that whenever we have C code run-around and generate a string to then pass to the interpreter to run, someone asks why we don't just make API calls directly, perhaps the answer is that we don't need to? >=20 > The other half of this is adding LIL features into regular commands. Th= e most > important feature here is being able to return a string result. I took = an > initial crack at it [3], but I think with this series there is a strong= er > motivating factor (along with things like [4]). >=20 > [3] https://patchwork.ozlabs.org/project/uboot/list/?series=3D231377 > [4] https://patchwork.ozlabs.org/project/uboot/list/?series=3D251013 >=20 > =3D Future work >=20 > The series as presented today is incomplete. The following are the major = issues > I see with it at the moment. I would like to address all of these issues,= but > some of them might be postponed until after first merging this series. >=20 > - There is a serious error handling problem. Most original LIL code never > checked errors. In almost every case, errors were silently ignored, even > malloc failures! While I have designed new code to handle errors proper= ly, > there still remains a significant amount of original code which just ig= nores > errors. In particular, I would like to ensure that the following catego= ries of > error conditions are handled: >=20 > - Running out of memory. > - Access to a nonexistant variable. > - Passing the wrong number of arguments to a function. > - Interpreting a value as the wrong type (e.g. "foo" should not have a = numeric > representation, instead of just being treated as 1). >=20 > - There are many deviations from TCL with no purpose. For example, the li= st > indexing function is named "index" and not "lindex". It is perfectly fi= ne to > drop features or change semantics to reduce code size, make parsing eas= ier, > or make execution easier. But changing things for the sake of it should= be > avoided. >=20 > - The test suite is rather anemic compared with the amount of code this > series introduces. I would like to expand it significantly. In particul= ar, > error conditions are not well tested (only the "happy path" is tested). >=20 > - While I have documented all new functions I have written, there are many > existing functions which remain to be documented. In addition, there is= no > user documentation, which is critical in driving adoption of any new > programming language. Some of this cover letter might be integrated wit= h any > documentation written. >=20 > - Some shell features such as command repetition and secondary shell prom= pts > have not been implemented. >=20 > - Arguments to native lil functions are incompatible with U-Boot function= s. For > example, the command >=20 > foo bar baz >=20 > would be passed to a U-Boot command as >=20 > { "foo", "bar", "baz", NULL } >=20 > but would be passed to a LIL function as >=20 > { "bar", "baz" } >=20 > This makes it more difficult to use the same function to parse several > different commands. At the moment this is solved by passing the command= name > in lil->env->proc, but I would like to switch to the U-Boot argument li= st > style. >=20 > - Several existing tests break when using LIL because they expect no outp= ut on > failure, but LIL produces some output notifying the user of the failure. >=20 > - Implement DISTRO_BOOT in LIL. I think this is an important proof-of-con= cept to > show what can be done with LIL, and to determine which features should = be > moved to LIL_FULL. >=20 > =3D Why Lil? >=20 > When looking for a suitable replacement shell, I evaluated implementation= s using > the following criteria: >=20 > - It must have a GPLv2-compatible license. > - It must be written in C, and have no major external dependencies. > - It must support bare function calls. That is, a script such as 'foo bar' > should invoke the function 'foo' with the argument 'bar'. This preserve= s the > shell-like syntax we expect. > - It must be small. The eventual target is that it compiles to around 10K= iB with > -Os and -ffunction-sections. > - There should be good tests. Any tests at all are good, but a functionin= g suite > is better. > - There should be good documentation > - There should be comments in the source. > - It should be "finished" or have only slow development. This will hopefu= lly > make it easier to port changes. On this last point, I believe this is based on lil20190821 and current is now lil20210502. With a quick diff between them, I can see that the changes there are small enough that while you've introduced a number of changes here, it would be a very easy update. > Notably absent from the above list is performance. Most scripts in U-Boot= will > be run once on boot. As long as the time spent evaluating scripts is kept= under > a reasonable threshold (a fraction of the time spend initializing hardwar= e or > reading data from persistant storage), there is no need to optimize for s= peed. >=20 > In addition, I did not consider updating Hush from Busybox. The mismatch = in > computing environment expectations (as noted in the "New shell" section a= bove) > still applies. IMO, this mismatch is the biggest reason that things like > functions and command substitution have been excluded from the U-Boot's H= ush. >=20 > =3D=3D lil >=20 > - zLib > - TCL > - Compiles to around 10k with no builtins. To 25k with builtins. > - Some tests, but not organized into a suite with expected output. Some e= vidence > that the author ran APL, but no harness. > - Some architectural documentation. Some for each functions, but not much. > - No comments :l > - 3.5k LoC >=20 > =3D=3D picol >=20 > - 2-clause BSD > - TCL > - Compiles to around 25k with no builtins. To 80k with builtins. > - Tests with suite (in-language). No evidence of fuzzing. > - No documentation :l > - No comments :l > - 5k LoC >=20 > =3D=3D jimtcl >=20 > - 2-clause BSD > - TCL > - Compiles to around 95k with no builtins. To 140k with builtins. Too big= =2E.. >=20 > =3D=3D boron >=20 > - LGPLv3+ (so this is right out) > - REBOL > - Compiles to around 125k with no builtins. To 190k with builtins. Too bi= g... >=20 > =3D=3D libmawk >=20 > - GPLv2 > - Awk > - Compiles to around 225k. Too big... >=20 > =3D=3D libfawk >=20 > - 3-clause BSD > - Uses bison+yacc... > - Awk; As it turns out, this has parentheses for function calls. > - Compiles to around 24-30k. Not sure how to remove builtins. > - Test suite (in-language). No fuzzing. > - Tutorial book. No function reference. > - No comments > - Around 2-4k LoC >=20 > =3D=3D MicroPython >=20 > - MIT > - Python (but included for completeness) > - Compiles to around 300k. Too big... >=20 > =3D=3D mruby/c >=20 > - 3-clause BSD > - Ruby > - Compiles to around 85k without builtins and 120k with. Too big... >=20 > =3D=3D eLua >=20 > - MIT > - Lua > - Build system is a royal pain (custom and written in Lua with external d= eps) > - Base binary is around 250KiB and I don't want to deal with reducing it >=20 > So the interesting/viable ones are > - lil > - picol > - libfawk (maybe) >=20 > I started with LIL because it was the smallest. I have found several > issues with LIL along the way. Some of these are addressed in this series > already, while others remain unaddressed (see the section "Future Work"). Thanks for the evaluations, of these, lil does make the most sense. --=20 Tom --J6AVObV96GkhyIUE Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQGzBAABCgAdFiEEGjx/cOCPqxcHgJu/FHw5/5Y0tywFAmDeI9wACgkQFHw5/5Y0 tyyYBgv/Q7huvHK4uNM0n8CBO6Ca43x934xHKQpsOxUbe48CQag16XmYKne73x3z k1OApbfbZJ12i/32WNaJd2mLbPhRzdzbu5wiGmKuCpvVryDGw09xjNEHmWVdukwb u9i3Urk6WsKjW+Q5MBQnDOb/NkPX3HubFmFaY3YxtptjcGCG+abDaExPoCTJHD6K xofroWvkAqBh3i9WNOcjs0icRsu+EHkUnQoXs8hXFp1xtMUoxOz625ipw0f1BNbu lRfsRJog7kv0IIDILN3UNmQlKzM8jHem6BUYKfya7DzHRTTjwz3lq+JcTkBXOOEs APCzJ22rI4J9QFh7ZdAAfTR/TfH6Qx3peXVWqOqIusfDGerwmayx1y5gtuX7iNVU jMx3hTt/DQCB+RUUMznjEnpLt1m3JdHuLo2s7jMS0QFBsNAJ7MwmHUQKAva93/4t McePOMD0TdpTSxnKMJSMqV52azBXZpJOzqPoPs3fLl3YiPfArCk5QsWvPL0qFeIc aVczjhh6 =3mNK -----END PGP SIGNATURE----- --J6AVObV96GkhyIUE--