Re: [PATCH v4] shell: exchange Dijkstra $(( )) evaluator..
On Wednesday, September 7, 2022, Steffen Nurpmeso
wrote:
> Kang-Che Sung wrote in
> :
> |On Wednesday, September 7, 2022, Steffen Nurpmeso
> |wrote:
> ...
> |>|> + if(su_64( i > U32_MAX || ) i >= UZ_MAX / 2 ||
> ...
> |>|I have to admit that the amount of macro maze makes it really hard to
> |>|read ;)
> |>
> |> Well it is easier than having lots of #ifdef #else #endif blocks
> |> in my opinion. On 32-bit the above would spit out a warning
> |> (possibly, i have not really looked what compiler/linker flags
> |> busybox uses) because i>U32_MAX can never be true. I mean i could
> ...
> |Compiler will optimize out always-false expressions without giving any
> |warning.
>
> Regarding the warning: not if you excess an integer limit.
> If you say if(0){} i go with you.
> Hm. Well, actually it seems current compilers really do so in
> simplemost test files at least, like
>
> int main(void){if(10>0xULL)return 1;return 0;}
>
> But already if it is a bit more complicated there is
>
> int main(void){
> unsigned long long int i = 10;
> if(i>0xULL)return 1;
> return 0;}
>
> t.c:3:6: warning: result of comparison 'unsigned long long' >
18446744073709551615 is always false [-Wtautological-type-limit-compare]
> if(i>0xULL)return 1;
> ~^~
> 1 warning generated.
>
> I can assure you that i am surprised, "in earlier times" you would
> have been thrown warnings at. (gcc 12.2.0 is silent!!!)
Okay, I didn't notice there is such a warning in Clang, but I would prefer
writing this way:
if ( (sizeof(i) > 4 && i > U32_MAX) || i >= UZ_MAX / 2 ...
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v4] shell: exchange Dijkstra $(( )) evaluator..
Kang-Che Sung wrote in
:
|On Wednesday, September 7, 2022, Steffen Nurpmeso
|wrote:
...
|>|> + if(su_64( i > U32_MAX || ) i >= UZ_MAX / 2 ||
...
|>|I have to admit that the amount of macro maze makes it really hard to
|>|read ;)
|>
|> Well it is easier than having lots of #ifdef #else #endif blocks
|> in my opinion. On 32-bit the above would spit out a warning
|> (possibly, i have not really looked what compiler/linker flags
|> busybox uses) because i>U32_MAX can never be true. I mean i could
...
|Compiler will optimize out always-false expressions without giving any
|warning.
Regarding the warning: not if you excess an integer limit.
If you say if(0){} i go with you.
Hm. Well, actually it seems current compilers really do so in
simplemost test files at least, like
int main(void){if(10>0xULL)return 1;return 0;}
But already if it is a bit more complicated there is
int main(void){
unsigned long long int i = 10;
if(i>0xULL)return 1;
return 0;}
t.c:3:6: warning: result of comparison 'unsigned long long' >
18446744073709551615 is always false [-Wtautological-type-limit-compare]
if(i>0xULL)return 1;
~^~
1 warning generated.
I can assure you that i am surprised, "in earlier times" you would
have been thrown warnings at. (gcc 12.2.0 is silent!!!)
|It's common for C code to produce many expressions like this that evaluate
|to constants (boolean or integer) at compile time as results of macro
|expansion, so no warnings should be given.
|
|The expression with a macro like `su_64( i > U32_MAX || )` looks really
|ugly to me. If what you are achieving is to remove statements that are
|always false (under certain preprocessor condition), just let the compiler
|do the job.
I do not think you are right here.
But sure, as the above is not critical at all, one could simply
use any other much, much lower limit, say, test the 32-bit
100.000.000 limit always, which makes the test expression much
easier. However i have found a bug in that
/* Done for empty expression */
if((i = a_shexp__arith_ws_squeeze(self, exp_buf, exp_len, NIL)) == 0)
goto jleave;
++i;
^ This could have overflowed before. It must be leftover, as we
++i later on (again), which is (then) safe. Ah, what a mess.
--steffen
|
|Der Kragenbaer,The moon bear,
|der holt sich munter he cheerfully and one by one
|einen nach dem anderen runter wa.ks himself off
|(By Robert Gernhardt)
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v4] shell: exchange Dijkstra $(( )) evaluator..
Steffen Nurpmeso wrote in <20220906193906.l5sy8%stef...@sdaoden.eu>: ... ||Missing license statement here? ... ||Ah, here is the license. Please move it to line 2 ? ... Btw i have no problem with relicensing this to Public Domain, or even simply removing the (ISC) license, shall it be integrated into math.c. It is more or less licensed only because of the disclaimer. --steffen | |Der Kragenbaer,The moon bear, |der holt sich munter he cheerfully and one by one |einen nach dem anderen runter wa.ks himself off |(By Robert Gernhardt) ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v4] shell: exchange Dijkstra $(( )) evaluator..
On Wednesday, September 7, 2022, Steffen Nurpmeso wrote: > |> + /* Overflow check: since arithmetic expressions are rarely \ > |> long enough > |> + * to come near this limit, xxx laxe & fuzzy, not exact; max \ > |> U32_MAX! */ > |> + if(su_64( i > U32_MAX || ) i >= UZ_MAX / 2 || > | > |I have to admit that the amount of macro maze makes it really hard to > |read ;) > > Well it is easier than having lots of #ifdef #else #endif blocks > in my opinion. On 32-bit the above would spit out a warning > (possibly, i have not really looked what compiler/linker flags > busybox uses) because i>U32_MAX can never be true. I mean i could > say i>=U32_MAX-1 or what, sure. The above is "laxe and fuzzy" > anyhow. :-) Compiler will optimize out always-false expressions without giving any warning. It's common for C code to produce many expressions like this that evaluate to constants (boolean or integer) at compile time as results of macro expansion, so no warnings should be given. The expression with a macro like `su_64( i > U32_MAX || )` looks really ugly to me. If what you are achieving is to remove statements that are always false (under certain preprocessor condition), just let the compiler do the job. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v4] shell: exchange Dijkstra $(( )) evaluator..
Hello.
Bernhard Reutner-Fischer wrote in
<20220906183821.1f82672d@nbbrfq>:
|On Wed, 31 Aug 2022 01:43:26 +0200
|Steffen Nurpmeso wrote:
|
|> The former implementation was not correct regarding whiteouts in
|> ?: conditional branches. The new one also parses a bit better, in
|> effect on equal level than bash with its recursive descendent parser.
|
|Please provide bloat-o-meter output.
|i.e.
|make baseline
|# apply patch
|make bloatcheck
That made me find docs/keep_data_small.txt.
With my own busybox .config (very much) against the same with also
CONFIG_FEATURE_SH_MATH_ERROR_TRACK=y, eh.. i get
/usr/bin/env: ‘python’: No such file or directory
make: *** [/x/src/busybox.git/Makefile.custom:112: bloatcheck] Error 127
There is only python3 here. I get
#?0|kent:busybox.git$ make bloatcheck
function old new delta
static.a_shexp__arith_eval -1891 +1891
a_shexp__arith_op_apply-1532 +1532
a_shexp__arith_val_eval- 272+272
arith 14 264+250
a_shexp_arith_op_toks - 199+199
strto_arith_t - 156+156
static.a_shexp__arith_ws_squeeze - 147+147
.rodata 104665 104750 +85
a_shexp__arith_op_apply_colons - 75 +75
ash_arith126 123 -3
op_tokens141 --141
arith_lookup_val 193 --193
arith_apply 1044 - -1044
evaluate_string 1146 - -1146
--
(add/remove: 7/4 grow/shrink: 2/1 up/down: 4607/-2527) Total: 2080 bytes
textdata bss dec hex filename
1926334 32428 29370 1988132 1e5624 busybox_old
1928414 32428 29370 1990212 1e5e44 busybox_unstripped
|> diff --git a/shell/math.c b/shell/math.c
|> index 76d22c9bd5..8ba0d2f7fb 100644
|> --- a/shell/math.c
|> +++ b/shell/math.c
|
|> +#define su_ienc_s64(X,Y,Z) (sprintf(X, ARITH_FMT, Y), X)
|> +#define n_var_vlook(X,Y) (*self->sac_cookie->lookupvar)(X)
|> +#define n_var_vset(X,Y,Z) (*self->sac_cookie->setvar)(X, (char*)(Y))
|> +#define su_idec_cp(A,B,C,D,E) a_idec_x(A, B, E)
|
|Can you drop the unused parameters?
Well if you busyboxify shexp-arith.h then yes.
|> +static inline uint32_t a_idec_x(void *resp, char const *cbuf,
|> + char const **endptr_or_nil){
|> + uint32_t rv;
|> + arith_t res;
|> + char const *eptr;
|> +
|> + if(endptr_or_nil == NULL)
|> + endptr_or_nil =
|
|Space after if please.
Below.
...
|> +++ b/shell/shexp-arith.h
|> @@ -0,0 +1,1292 @@
|> +/*@ S-nail - a mail user agent derived from Berkeley Mail.
|
|Missing license statement here?
That is not always in line 2 for busybox, too.
...
|> + *@ - a_SHEXP_ARITH_COMPAT_SHIMS: for inclusion in other code bases, \
...
|these defines
Yes?
...
|Ah, here is the license. Please move it to line 2 ?
...
|What's NYD2_IN supposed to accomplish? Delete.
So what is this now? I said shexp-arith.h is taken as-is from my
mailer for which i wrote it. It could remain so, hooked in via
compatibility shims, or the code may be moved entirely to
busybox's math.c, with syntax adjustments just as you desire.
In this case i would expect the above bloatcheck to become
less of a headache. A bit at least.
NYD is part of my infrastructure that is used in shexp-arith.h.
|> + /* Overflow check: since arithmetic expressions are rarely \
|> long enough
|> + * to come near this limit, xxx laxe & fuzzy, not exact; max \
|> U32_MAX! */
|> + if(su_64( i > U32_MAX || ) i >= UZ_MAX / 2 ||
|
|I have to admit that the amount of macro maze makes it really hard to
|read ;)
Well it is easier than having lots of #ifdef #else #endif blocks
in my opinion. On 32-bit the above would spit out a warning
(possibly, i have not really looked what compiler/linker flags
busybox uses) because i>U32_MAX can never be true. I mean i could
say i>=U32_MAX-1 or what, sure. The above is "laxe and fuzzy"
anyhow. :-)
Btw if you start reading after the /* -- >8 -- 8< -- */ scissor
line it is quite in a flow. (I usually do not use preprocessor
macro in files, the support uses quite a lot, though.)
I mean, i have a little infrastructure, which is used here.
The pure part of it, and i am hoping the mailer part of it, to
which shexp-arith.h belongs, in a distant future, too, can be
compiled with a C++ compiler also.
Regarding overall syntax: is there a formatter
Re: [PATCH v4] shell: exchange Dijkstra $(( )) evaluator..
Hi Steffen,
On Wed, 31 Aug 2022 01:43:26 +0200
Steffen Nurpmeso wrote:
> The former implementation was not correct regarding whiteouts in
> ?: conditional branches. The new one also parses a bit better, in
> effect on equal level than bash with its recursive descendent parser.
Please provide bloat-o-meter output.
i.e.
make baseline
# apply patch
make bloatcheck
> diff --git a/shell/math.c b/shell/math.c
> index 76d22c9bd5..8ba0d2f7fb 100644
> --- a/shell/math.c
> +++ b/shell/math.c
> +#define su_ienc_s64(X,Y,Z) (sprintf(X, ARITH_FMT, Y), X)
> +#define n_var_vlook(X,Y) (*self->sac_cookie->lookupvar)(X)
> +#define n_var_vset(X,Y,Z) (*self->sac_cookie->setvar)(X, (char*)(Y))
> +#define su_idec_cp(A,B,C,D,E) a_idec_x(A, B, E)
Can you drop the unused parameters?
> +
> +static inline uint32_t a_idec_x(void *resp, char const *cbuf,
> + char const **endptr_or_nil){
> + uint32_t rv;
> + arith_t res;
> + char const *eptr;
> +
> + if(endptr_or_nil == NULL)
> + endptr_or_nil =
Space after if please.
> diff --git a/shell/shexp-arith.h b/shell/shexp-arith.h
> new file mode 100644
> index 00..5f3655b456
> --- /dev/null
> +++ b/shell/shexp-arith.h
> @@ -0,0 +1,1292 @@
> +/*@ S-nail - a mail user agent derived from Berkeley Mail.
Missing license statement here?
> + *@ Signed 64-bit sh(1)ell-style $(( ARITH ))metic expression evaluator.
> + *@ POW2 bases are parsed as unsigned, operation overflow -> limit
> constant(s),
> + *@ saturated mode is not supported, division by zero is handled via error.
> + *@ The expression length limit is ~100.000.000 on 32-bit, U32_MAX otherwise.
> + *@ After reading on Dijkstra's two stack algorithm, as well as bash:expr.c.
> + *@ Most heavily inspired by busybox -- conclusion: the Dijkstra algorithm
> + *@ scales very badly to ternary as are used to implement conditionals and
> + *@ their ignored sub-expressions.
> + *@
> + *@ #define's:
> + *@ - a_SHEXP_ARITH_COMPAT_SHIMS: for inclusion in other code bases, setting
> + *@ this defines most necessary compat macros.
these defines
> + *@ We still need s64, u64, S64_MIN, savestr(CP) <> strdup(3) that does not
> + *@ return NIL (only with _ERROR_TRACK). Plus stdint.h, ctype.h, string.h.
> + *@ We need su_idec_cp(), su_ienc_s64(), n_var_vlook() and n_var_vset().
> + *@ We need su_IDEC_STATE_EMASK (= 1) and su_IDEC_STATE_CONSUMED (= 2),
> e.g.:
> + *@ errno = 0;
> + *@ res = strto_arith_t(cbuf, (char**)endptr_or_nil);
> + *@ rv = 0;
> + *@ if(errno == 0){
> + *@ if(**endptr_or_nil == '\0')
> + *@ rv = su_IDEC_STATE_CONSUMED;
> + *@ }else{
> + *@ rv = su_IDEC_STATE_EMASK;
> + *@ res = 0;
> + *@ }
> + *@ *S(s64*,resp) = res;
> + *@ - a_SHEXP_ARITH_COOKIE: adds struct a_shexp_arith_ctx:sac_cookie, and
> + *@ a cookie arg to a_shexp_arith_eval().
> + *@ - a_SHEXP_ARITH_ERROR_TRACK: add "char **error_track_or_nil" to
> + *@ a_shexp_arith_eval(), and according error stack handling, so that users
> + *@ can be given hint where an error occurred. ("Three stack algorithm.")
> + *
> + * Copyright (c) 2022 Steffen Nurpmeso .
> + * SPDX-License-Identifier: ISC
> + *
> + * Permission to use, copy, modify, and/or distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
Ah, here is the license. Please move it to line 2 ?
> +static void
> +a_shexp__arith_eval(struct a_shexp_arith_ctx *self,
> + char const *exp_buf, uz exp_len){
> + char *ep, *varp, *cp, c;
> + u16 lop;
> + struct a_shexp_arith_stack *sasp;
> + void *mem_p;
> + NYD2_IN;
What's NYD2_IN supposed to accomplish? Delete.
> +
> + a_SHEXP_ARITH_L((" > _arith_eval %zu <%.*s>\n",
> + exp_len, S(int,exp_len != UZ_MAX ? exp_len : su_cs_len(exp_buf)),
> + exp_buf));
> +
> + mem_p = NIL;
> + sasp = self->sac_stack;
> +
> + /* Create a single continuous allocation for anything */
> + /* C99 */{
> + union {void *v; char *c;} p;
> + uz i, j, a;
> +
> + /* Done for empty expression */
> + if((i = a_shexp__arith_ws_squeeze(self, exp_buf, exp_len, NIL)) == 0)
> + goto jleave;
> + ++i;
> +
> + /* Overflow check: since arithmetic expressions are rarely long enough
> + * to come near this limit, xxx laxe & fuzzy, not exact; max U32_MAX!
> */
> +
