Should we delete unused libast stuff?

[McDutchie]

For a while in 2020/2021, there seemed to be some interest in our fork of libast, the library used by ksh93, as a separate entity that could perhaps be used by other projects. If I'm not mistaken, I think it was @lkujaw who was interested in that, but he now has his own AST fork (which has since gone inactive) and seems to have lost interest.

Meanwhile, bit rot looms; stuff that is not used is untested and may break because of other changes we make. It's also dead weight that is getting compiled, but not used.

One example is the obsolete hash(3) library which has been replaced by cdt(3). There's also the magic(3) library which is used by the AST version of the file command, which we're not shipping. And I'm sure the list could be made much longer; I haven't had the time or motivation to look into it so far, as there are still (!) far too many bugs in basic ksh93 functionality.

Graphviz is also a user of libast, but they've stripped it down so much there is barely anything left. I don't intend to be that radical. But I don't want a lot of bit rot and dead weight either. That's an AT&T tradition I would not care to continue :)

On the other hand, at some point I'm going to get around to re-enabling the ability to build dynamic libraries (see the currently dormant #357, which at this point I'll have to port to the current code). At that point it will become possible to write other programs to dynamically link against libast, and hypothetically, someone could want some of that stuff we're not using.

Thoughts, opinions?

Ping @JohnoKing, @hyenias, any other interested party

[lkujaw]

Hi,

It makes sense to drop unrelated code that detracts from maintaining ksh, but I think it might help to clarify the last paragraph. Are you planning on shipping libast as AT&T intended, a sort of grab bag for useful functionality absent from libc, or just an auxiliary library for ksh? If it is the latter, you might want to consider rebranding it libksh93 or something to avoid linking conflicts.

[McDutchie]

It makes sense to drop unrelated code that detracts from maintaining ksh, but I think it might help to clarify the last paragraph. Are you planning on shipping libast as AT&T intended, a sort of grab bag for useful functionality absent from libc, or just an auxiliary library for ksh?

Well, that is just the core question I would like community input on.

Since you're here -- are you still likely to have an interest in our libast version or the many fixes in it at some point?

If it is the latter, you might want to consider rebranding it libksh93 or something to avoid linking conflicts.

I'm afraid we're already past the point of ABI compatibility, so linking conflicts should be avoided in any case. We should still be 99.9% source-compatible though, so I'm not sure about renaming it just yet.

[lkujaw]

Well, that is just the core question I would like community input on.

Since you're here -- are you still likely to have an interest in our libast version or the many fixes in it at some point?

Absolutely. I think the best approach would be for me to dig through ksh93 project history to cherry pick fixes, because the path to adapting either the Toolchest or the project to use a common libast is not clear to me and it seems fair that the project using libast's more esoteric features should bear the burden of maintaining them.

If it is the latter, you might want to consider rebranding it libksh93 or something to avoid linking conflicts.

I'm afraid we're already past the point of ABI compatibility, so linking conflicts should be avoided in any case. We should still be 99.9% source-compatible though, so I'm not sure about renaming it just yet.

The only concern that I had was a scenario in which packagers decide to ship ksh93 with a dynamic libast. Depending on the system configuration, SOVERSION, etc., it could make using the Toolchest a rather frustrating experience.

[JohnoKing]

Use of libast as a dependency is uncommon. The only applications I'm aware of that use it are the following:

• ksh93
• CDE's dtksh
• ksh93 custom builtins (such as ksh93-match)
• graphviz
• AT&T AST (i.e., AT&T pax, nmake, ndfs, etc.)
• AT&T VizGEMS
• UWIN (via ksh93)

Of the above applications, many already roll their own versions of libast, and cannot use the 93u+m version. The only ones for which compatibility is a relevant concern are dtksh and custom builtins. Functionality in libast that is used by neither 93u+m nor dtksh is most likely not being used by anything else. The vast majority of the currently unused libast code is only used by various tools in the AST toolkit such as file and nmake; no one's actually trying to use those with 93u+m's version of libast, which make compatibility with them a moot point. Code that is only used by the AST toolkit is not going to be used for anything else and should be removed.

[hyenias]

Agreed. My interest aligns with creating more ksh93 custom builtins to further provide a 3rd party standard library: ini/csv/json/etc., sqlite, array sorting, zstd strings, big_int, libmawk, lmdb/unqlite. Additionally, just remotely maybe breaking out ksh's math functions into autoloaded functions via an externally dynamic loadable library would benefit ksh's memory foot print and file size. Perhaps the help strings could be placed in an external file to further reduce code size until needed.

[posguy99]

At least on macOS, Ksh93u+m is already smaller than any of the shipped shells save for tcsh and dash.

[JohnoKing]

Also, I should separately add that the majority of the vmalloc code is obsolete and isn't currently being used at all. The original reason it was kept was so that the memory leak regression tests could continue working via the vmstate builtin. Those usually run with standard malloc compatible tools nowadays, and I should also note that ksh now can compile with ASan, which is capable of detecting memory leaks. Vmalloc is thus effectively unused and should be removed.
Granted, some of the vmalloc wrapper code is still being used and for now must be kept in place to prevent the introduction of new memory leaks. (Most notably, vmclose doesn't appear to have a standard malloc equivalent afaik.) An initial removal should simply remove everything not enabled by #ifdef _std_malloc, as well as remove vmstate.

[McDutchie]

Maybe I'm being thick, but I don't get it. Your first bullet point and your third seem to contradict each other. How is it not using the native malloc if _std_malloc is enabled?

[JohnoKing]

Adding additional clarification: The code in getmemory is supposed to be used when _std_malloc is defined. The very existence of the mallocmem function proves this, since it's only compiled in when _std_malloc is enabled. However, the getmemory, for whatever reason, is never reached while _std_malloc is used. If all of the code in getmemory is replaced by abort();, nothing happens. All regression tests pass without any crashes. This indicates getmemory is never used with _std_malloc. However, it is reached with vmalloc: an inserted abort will actually crash ksh when _AST_vmalloc enabled.

Additionally, even if getmemory were to be reached with _std_malloc on, getmemory() would still avoid malloc at any cost. The code is structured in such a way that malloc isn't reached unless mmap and sbrk both error out, usually with ENOMEM. At that point attempting malloc is pointless, since there's no memory available to allocate. (Additionally, the sbrk and mmap methods are compiled in and used on systems that support it, regardless of _std_malloc's status.)

[McDutchie]

Thanks, got it. Good grief. :/

So, do you think this is a release-blocking issue?

[JohnoKing]

Thankfully, the answer to that is no. Ksh is currently using standard malloc for allocation (hence why ASan and memory allocation works at all). That's because the broken getmemory function is never reached, which means mmap/sbrk are inadvertently denied the opportunity to override malloc. The code in question is still highly broken though and could cause problems in the future, so I'd say that this is certainly at the top of the list for things to remove in 1.1 (along with many other messy parts of vmalloc).

[McDutchie]

The code in getmemory is supposed to be used when _std_malloc is defined. The very existence of the mallocmem function proves this, since it's only compiled in when _std_malloc is enabled.

My impression is that the _std_malloc flaggery looks rather incomplete and seems to make no real effort to avoid compiling in unused code, so I wouldn't necessarily take that as proof of anything.

It would seem to me that getmemory() is really only supposed to be used if vmalloc is enabled. It would make sense for vmalloc to use low-level syscalls and avoid malloc. So when _std_malloc is enabled, that's dead code.

Adding a sfprintf(sfstderr,"[DEBUG] getmemory\n"); to that function and testing ksh compiled with and without -D_AST_vmalloc seems to confirm that.

[ormaaj]

On the other hand, at some point I'm going to get around to re-enabling the ability to build dynamic libraries (see the currently dormant #357, which at this point I'll have to port to the current code). At that point it will become possible to write other programs to dynamically link against libast, and hypothetically, someone could want some of that stuff we're not using.

I'm particularly interested in this. libast would be a very valuable tool in this case, but indeed #357 is a huge barrier. Limping along with these arcane hand-coded mamake files is rough. :/

[McDutchie]

I'm particularly interested in this. libast would be a very valuable tool in this case, but indeed #357 is a huge barrier.

It's good to know you're interested. This significantly raises the issue on my priority list.

I already had #357 working on i386/x86_64, but it crashes on ARM, which is clearly unacceptable; that needs to be fixed before merging it. You can test my dyn branch right now – you'll just be using a 2021 version of ksh and all the surrounding code.

There have been comprehensive changes since then, so now the PR needs to be ported to the current code, but that should be relatively easy. I haven't gotten to it so far because nobody actually seemed to be asking for this, and because there were (and still are) so many high-priority bugs to fix in basic ksh functionality.

But that's no huge barrier to porting #357 so we can test it on the current code; I just need to get motivated to spend a few hours on it.

Limping along with these arcane hand-coded mamake files is rough. :/

In fact, now that I've made a few tweaks (such as indentation), I've been finding it remarkably easy. There are five commands and six attributes. The basic unit is a make...done block. A nested make...done block creates a dependency. prev commands refer to previous make...done blocks or (as of recently) to prerequisite files. exec - adds a line to a shell script for the current block which will be executed upon reaching done. It is a very simple language and it shouldn't take more than about half an hour to learn it.

Maintaining them does involve moving those blocks around and fixing their indentation, and judicious use of search/replace operations as there is a lot of repetition, but that's nothing a good editor cannot handle. The main problem is the unreadable generated shell code in the exec - scripts. But when you look up the sources in the original nmake Makefiles and rule dependencies – okay, there's indentation in the original shell code there, but otherwise it's equally arcane. So I've been gradually fixing all that, and this will continue.

I really tried to stick with the full nmake back in 2020, but it simply wasn't going to happen, because it never worked at all on macOS and really wasn't working properly on anything. It is a behemoth that is just as complex and buggy as ksh itself was, because there were no quality standards at this AT&T team, they were always forgetting about existing projects and chasing the lastest shiny research ideas, and they'd forgotten all about the KISS principle. Absolutely nobody has time to fix nmake. As for me, I am using all my available time on just ksh and a few of its peripherals, and you've seen how slow progress has been as it is.

Whereas mamake is a portable C90 program, written in one file of about 2500 lines. It is very simple. And even that had a few significant bugs in it, but it is simple enough we were able to fix them. There was no real documentation, but it is simple enough I was able to write it from reading the source code.

And it's very portable, so now we can still build on AIX, HP-UX, Solaris, QNX, etc. - the only dependencies that this build system has is a POSIX-compliant userland and C compiler, for very loose values of POSIX-compliant. That's a major advantage, and not something I want to give up.

The fact that building dynamic libraries, changing SHOPT_* compile-time options, running the regression tests, running 'bin/package install', etc., all depended on nmake, was purely a choice on the part of the AST team. mamake easily runs arbitrary shell code, so its flexibility is unlimited. And all those things are already reimplemented.

I just need to port the dynamic library handling code to the current dev branch, and then we need to find a way to fix the cause of it crashing on ARM. That has been the real barrier to merging it all along, and it's got nothing to do with mamake.

[ormaaj]

thank you for your many detailed responses. I do try to study your detailed analysis. :)