Re: [PATCH master] Introduce libmescc.a; Put division by integer in there; split syscalls' errno off.

[Jan Nieuwenhuizen]

Danny Milosavljevic writes:

> * build-aux/configure-lib.sh (libmescc_SOURCES): Add lib/mes/div0.c, 
> lib/mes/div.c, lib/linux/*/syscall-internal.c.
> * build-aux/build-lib.sh: Add libmescc.a.
> * build-aux/build-mes.sh: On gcc, add "-lmescc".
> * build-aux/test-c.sh: Add "-lmescc".
> * build-aux/check.sh.in: Add mescc to LIBS.
> * module/mescc/mescc.scm (mescc:link): Add "mescc".
> * module/mescc.scm (mescc:main): Update documentation of "-nodefaultlibs" and 
> "-nostdlib".
> * lib/mes/div.c (ldiv): Avoid assert.

Being very nitpicky here, could you hit M-q to break some of these long
lines, like so:

* build-aux/configure-lib.sh (libmescc_SOURCES): Add lib/mes/div0.c,
lib/mes/div.c, lib/linux/*/syscall-internal.c.
* build-aux/build-lib.sh: Add libmescc.a.
* build-aux/build-mes.sh: On gcc, add "-lmescc".
* build-aux/test-c.sh: Add "-lmescc".
* build-aux/check.sh.in: Add mescc to LIBS.
* module/mescc/mescc.scm (mescc:link): Add "mescc".
* module/mescc.scm (mescc:main): Update documentation of
"-nodefaultlibs" and "-nostdlib".
* lib/mes/div.c (ldiv): Avoid assert.


> * build-aux/build-lib.sh: Add libmescc.a.
> * build-aux/build-mes.sh: On gcc, add "-lmescc".
> * build-aux/test-c.sh: Add "-lmescc".
> * build-aux/check.sh.in: Add mescc to LIBS.
> * module/mescc/mescc.scm (mescc:link): Add "mescc".
> * module/mescc.scm (mescc:main): Update documentation of "-nodefaultlibs" and 
> "-nostdlib".
> * lib/mes/div.c (ldiv): Avoid assert.
> (__mesabi_div0): Avoid assert.
> (__aeabi_idivmod): New procedure.
> (__aeabi_idiv): New procedure.
> (__aeabi_uidivmod): New procedure.
> (__aeabi_uidiv): New procedure.
> (__mesabi_div0): Move to...
> * lib/mes/div0.c (__mesabi_div0): ...here.  Use "__raise".
> * lib/linux/x86-mes-gcc/syscall.c (__sys_call, __sys_call1, __sys_call2, 
> __sys_call3, __sys_call4): Move to...
> * lib/linux/x86-mes-gcc/syscall-internal.c: ...here.
> (__raise): New procedure.
> * lib/linux/x86-mes-mescc/syscall.c (__sys_call, __sys_call1, __sys_call2, 
> __sys_call3, __sys_call4): Move to...
> * lib/linux/x86-mes-mescc/syscall-internal.c: ...here.
> (__raise): New procedure.

Likewise.

[..]

>  if [ -z "${i/[012][0-9]-*/}" ]; then
> -    LIBS=
> +    LIBS='-l mescc'

I didn't see this before, yes this is very nice!

[..]

> diff --git a/lib/mes/div.c b/lib/mes/div.c
> index fa61a691..2caec981 100644
> --- a/lib/mes/div.c
> +++ b/lib/mes/div.c
> @@ -20,7 +20,6 @@
>   */
>  
>  #include <mes/lib.h>
> -#include <assert.h>
>  #include <stdint.h>
>  #include <limits.h>
>  
> @@ -32,14 +31,17 @@ typedef struct
>    long rem;
>  } ldiv_t;
>  
> +
>  void __mesabi_div0(void)

Please use GNU coding style, like so

void
__mesabi_div0 (void)

(line break and space).  This is an oversight, right; you've been doing
this nicely everywhere.


> -/* Compare gcc: __udivdi3 */
> +/* Compare gcc: __udivmoddi4 */
>  unsigned long __mesabi_uldiv(unsigned long a, unsigned long b, unsigned 
> long* remainder)

Same here.

> +#if __GNUC__ && !SYSTEM_LIBC && __arm__
> +// /gnu/store/7sfr3vhxq7l4mai8m0fr1cd8w9xcj9dh-binutils-2.31.1/bin/ld: 
> hash.o: in function `hash_cstring':

I think we should avoid adding store paths or rather, not use them at
all.  How about this

  // ...-binutils-2.31.1/bin/ld: hash.o: in function `hash_cstring':

?

> +// hash.c:(.text+0x56): undefined reference to `__aeabi_idivmod'
> +// /gnu/store/7sfr3vhxq7l4mai8m0fr1cd8w9xcj9dh-binutils-2.31.1/bin/ld: 
> math.o: in function `divide':
> +// math.c:(.text+0x516): undefined reference to `__aeabi_idiv'
> +// /gnu/store/7sfr3vhxq7l4mai8m0fr1cd8w9xcj9dh-binutils-2.31.1/bin/ld: 
> math.o: in function `modulo':
> +// math.c:(.text+0x5d2): undefined reference to `__aeabi_idivmod'
> +// /gnu/store/7sfr3vhxq7l4mai8m0fr1cd8w9xcj9dh-binutils-2.31.1/bin/ld: 
> gcc-lib/libc.a(ntoab.o): in function `ntoab':
> +// ntoab.c:(.text+0x54): undefined reference to `__aeabi_uidivmod'
> +// /gnu/store/7sfr3vhxq7l4mai8m0fr1cd8w9xcj9dh-binutils-2.31.1/bin/ld: 
> ntoab.c:(.text+0x62): undefined reference to `__aeabi_uidiv'


> +/* Result: r0: quotient; r1: remainder */
> +long
> +__aeabi_idivmod (long a, long b)
> +{
> +  ldiv_t result = ldiv(a, b);
> +  register long rem_result asm("r1");
> +  rem_result = result.rem;
> +  return result.quot;
> +}
> +
> +long
> +__aeabi_idiv (long a, long b)
> +{
> +  ldiv_t result = ldiv(a, b);
> +  return result.quot;
> +}
> +
> +/* Result: r0: quotient; r1: remainder */
> +unsigned long
> +__aeabi_uidivmod (unsigned long a, unsigned long b)
> +{
> +  unsigned long quot;
> +  unsigned long rem;
> +  register unsigned long rem_result asm("r1");
> +  quot = __mesabi_uldiv (a, b, &rem);
> +  rem_result = rem;
> +  return quot;
> +}
> +
> +unsigned long
> +__aeabi_uidiv (unsigned long a, unsigned long b)
> +{
> +  return __mesabi_uldiv (a, b, 0);
> +}
> +#endif // __GNUC__ && !SYSTEM_LIBC && __arm__
> diff --git a/lib/mes/div0.c b/lib/mes/div0.c

You said div0 was not needed anymore, right?

Otherwise, LGTM (looks truly great to me, awesome)!

Greetings,
Janneke

-- 
Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyofSource.com | Avatar® http://AvatarAcademy.com