[illumos-Developer] Round eleven of code review for ksh93-integration+POSIX utility update3 ...

Tue Apr 5 17:56:12 PDT 2011

I originally sent this on March 21 but it ended up in the list moderator's
queue due to size, with apparently no easy way to get it sent.  So I'm
re-sending with the attachments replaced with links.  See:

  http://mexico.purplecow.org/gwr/ksh93/eight-to-four.txt
  http://mexico.purplecow.org/gwr/ksh93/four-to-one.txt
  http://mexico.purplecow.org/gwr/ksh93/move-to-arch.txt
  http://mexico.purplecow.org/gwr/ksh93/re-combine.patch

Sorry about the delay.
Gordon

---------- Forwarded message ----------
From: Gordon Ross <gordon.w.ross at gmail.com>
Date: 2011/3/21
Subject: Re: [illumos-Developer] Fwd: [Urgend] Round eleven of code
review for ksh93-integration+POSIX utility update3 ...
To: ольга крыжановская <olga.kryzhanovska at gmail.com>
Cc: illumos-dev <developer at lists.illumos.org>

2011/3/17 ольга крыжановская <olga.kryzhanovska at gmail.com>:
> Illumos developers, I forward the code review request for the POSIX
> utility (aka posixcore) and ksh update 3 to gather feed back from the
> Illumos developers.
> This is the same patch set which is used for Oracle Solaris 11; we
> hope to use almost identical patches for Illumos and Oracle Solaris
> for this time.

That's reassuring to know, but please understand that illumos is a
separate project from Solaris or OpenSolairs.

Also, this is not "round eleven" for most illumos reviewers.

> Code review for Oracle Solaris 11 was done by:
[...]
> I set the time out to Saturday, 19.3.2011, because the code has
> significant coverage and the put back already approved for Oracle
> Solaris.
>
> Olga
>
> ---------- Forwarded message ----------
> From: Roland Mainz <roland.mainz at nrubsig.org>
> Date: Fri, Feb 11, 2011 at 4:23 PM
> Subject: [Urgend] Round eleven of code review for
[...]
> I'd like to request another code review for the upcoming
> ksh93-integration+POSIX utility update3 putback. I hope this is the
> final round now that all other feedback has been integrated and the
> codebase has been updated to match ast-ksh.2011-02-08.
>
>
> Webrev can be found at
> http://cr.opensolaris.org/~gisburn/on_posix_utility_update2_201100211_017/
[...]

> The hg export file is available at
> http://cr.opensolaris.org/~gisburn/on_posix_utility_update2_201100211_017/on_ksh93_ast_utility_update2_hg_export_20110211_017.hgexport.bz2

*gwr1: Why generated code?

Why do we have "generated" source checked in for ksh93?
Could we not check in the original and generate during build?

I'm told that the AST original source uses some sort of
"autoconf" like tool, which might make the generated source
vary more than we like (based on configuration options or
conditions on the build machine, etc.)  So, rather then run
the full AST autoconfig tool (iffe?) perhaps some simplified
alternative could be used to convert the original AST code
into something like the "merged" sources described below?

My concern is that, checking in generated code makes it
much harder for people to maintain this code and/or send
their bug fixes "upstream".

*gwr2: Why so much duplicated file content?

Many code sections exist in eight files, i.e.:

 $SRC/usr/src/lib/libast/amd64/include/ast/ast_api.h
 $SRC/usr/src/lib/libast/amd64/src/lib/libast/ast_api.h
 $SRC/usr/src/lib/libast/i386/include/ast/ast_api.h
 $SRC/usr/src/lib/libast/i386/src/lib/libast/ast_api.h
 $SRC/usr/src/lib/libast/sparc/include/ast/ast_api.h
 $SRC/usr/src/lib/libast/sparc/src/lib/libast/ast_api.h
 $SRC/usr/src/lib/libast/sparcv9/include/ast/ast_api.h
 $SRC/usr/src/lib/libast/sparcv9/src/lib/libast/ast_api.h

Same for ast_stdio.h (as richlowe observed) and many others.
See the attached eight-to-four.txt for the full list.
[ http://mexico.purplecow.org/gwr/ksh93/eight-to-four.txt ]

Many more code sections exist in four files, i.e.
See the attached four-to-one.txt for the full list.
[ http://mexico.purplecow.org/gwr/ksh93/four-to-one.txt ]

This issue appears in most AST-provided libraries:
 $SRC/lib/libast
 $SRC/lib/libsum
 $SRC/lib/libcmd
 $SRC/lib/libdll
 $SRC/lib/libshell
 $SRC/lib/libpp

We really should merge the headers before putting under
SCM, and get rid of the hacks for install_h in libast.  I would
do this both for the headers that are identical (many are)
and those that vary some across architectures.  The latter
can easily be merged using preprocessor conditionals, as is
demonstrated in the patch below.

To help narrow down what needs review, I took a stab at
"folding together" those duplicate headers. I've attached
four files detailing the steps I did to combine these:
 eight-to-four.txt :   Remove all of these, going
                       from eight copies to four.
 four-to-one.txt :     Remove these too, going from
                       four copies to one.
 move-to-arch.txt :    Move remaining i386 files to "arch".
                       Could put these under common instead,
                       but this made the experiment easy.
 re-combine.patch :    Patch the remaining files to allow
                       their use for all architectures, and
                       update Makefiles, etc accordingly.
[ http://mexico.purplecow.org/gwr/ksh93/move-to-arch.txt
  http://mexico.purplecow.org/gwr/ksh93/re-combine.patch ]

I don't intend to suggest that you need to combine these
duplicated headers exactly the way I've done, but I would
request that you come up with _some_ way to merge them.
My experiment demonstrates that it's not hard to do.

Perhaps you can find a way to make the code that generates
these files produce the merged form.  Or perhaps you might
add a post-processing step to get them into merged form.

In any case, please choose a method and merge these before
checking them in as SCM files.

*gwr3: Some per-arch. variations look like bugs.

After examining the variations that remain after merging
the duplicate header files, some variations look like bugs.
These could be bugs in the tool that generates this code.

I've marked all of these with XXX in the attached patch.
They are described in more detail here:

Signal lists are wrong for sparc in:
 $SRC/lib/libast/sparc/src/lib/libast/FEATURE/signal
 $SRC/lib/libshell/arch/src/cmd/ksh93/FEATURE/sigfeatures
These should not vary per architecture.

In $SRC/lib/libast/$MACH/include/ast/ast_common.h the macros
va_listref(), va_listval(), va_listarg have a different form
on amd64.  This looks suspicious. Code using these macros has
questionable portability.

Is C99 stdarg functionality not sufficient for what
this code needs to do?  (i.e. va_copy(), etc.)
These are used in libast:common/string/tokscan.c
and libast:common/hash/hashalloc.c and it would
appear that one should be able to make that code
work without va_listarg etc.

In $SRC/lib/libast/$MACH/include/ast/ast_dirent.h
there is some questionable code, conditionally compiled
on 32-bit platforms, to make libast use 64-bit offsets.
To make off64_t exposed, define _LARGEFILE64_SOURCE=1,
then just use the off64_t type.  Further, instead of the
define of off_t to this type, why doesn't this code use
some typedef of its own? i.e. typedef off64_t ast_off_t ?
Same goes for the defines of dirent and readdir.
There are "cleaner" ways to get 64-bit file APIs in
both 32-bit/64-bit builds on illumos or Solaris.

In $SRC/lib/libast/$MACH/include/ast/ast_fs.h
These defines are only in 32-bit builds.  Why?
 #define _lib__fxstat   1       /* _fxstat() in default lib(s) */
 #define _lib__lxstat   1       /* _lxstat() in default lib(s) */
 #define _lib__xmknod   1       /* _xmknod() in default lib(s) */
 #define _lib__xstat    1       /* _xstat() in default lib(s) */
 #define _ary_st_pad4   1       /* stat.st_pad4 is an array */

In $SRC/lib/libast/$MACH/include/ast/ast_sizeof.h
The 32-bit build defines:
#define _ast_sizeof_intmax_t    8
Should that be 4?

In $SRC/lib/libast/$MACH/include/ast/ast_stdio.h
Looks like FOPEN_MAX has needless variation across arch.
(Was 60 on i386 and 20 on all others.  Why?)

Thanks,
Gordon