Inconsistencies in Maiko command line arg processing for sysouts

[fghalasz]

In Maiko, processing of command line arguments for sysout has lots of inconsistencies.

1. The -sysout argument can only be used when using X Windows. On non-X platforms, there is no -sysout argument.

2. Bug: On X windows only: if 1) the sysout is specified as the first argument to lde, 2) there are additional command line arguments, 3) the env variables LDESOURCESYSOUT and LDESRCESYSOUT are not set, and 4) there is no file $HOME/lisp.virtualmem, THEN lde will immediately exit with a "can't find a sysout" error message - despite the fact that the sysout given as the first command line arg is a valid syspout file. This bug does not happen on SDL platforms - e.g., cygwin.

3. In general, sysout args are processed twice on X-windows platforms - once in xrdopt.c (specific to X) and once in main.c (for all platforms). The processing in these two places is both redundant and, more importantly, inconsistent. The above bug is caused by faulty processing of the sysout argument in the xrdopt.c code (looks at the first command line argument only if argc == 2).

[nbriggs]

2. I don't see that --

 % rm ~/lisp.virtualmem   
rm: /Users/briggs/lisp.virtualmem: No such file or directory
briggs@macrobiotic medley % lde loadups/lisp.sysout -title "FOO"

starts up fine with the sysout (using ldex), producing an X window with title "FOO". Neither LDESOURCESYSOUT nor LDESRCESYSOUT were set. Can you give me one with different arguments that exhibits the failure.

3. I agree - but I don't have a good design in mind for how to make a simple enough but extensible parsing setup which can cope with display subsystem specific options (e.g., X11 and SDL may have different requirements) and also incorporate X11's desire to parse all the options itself. I don't know where the -sysout option came from.

[nbriggs]

Re: the -sysout - it looks as though it came about so that you can specify the ldex*sysout X resource in your default X resource database. I wouldn't expect it to be used. The sysout name gets set by the code in main.c (common for X11 and SDL) line 371, where it may not have been set by the X11 options parser (if more than 1 argument) in:

  if (argc > 1 && argv[1][0] != '-') {
    strncpy(sysout_name, argv[1], MAXPATHLEN);
    i++;

[fghalasz]

@nbriggs One more condition for bug to appear: you cannot have a sysout specified in any of the X resource databases (.Xresources etc).

Check out code in xrdopt.c: Lines 184-198 and then Lines 246-253 (in master). Given all of the conditions I specified, the check starting at line 250 will result in an immediate error exit before the first sysout argument is processed.

[nbriggs]

Except it doesn't. From the XrmParseCommand() man page --

       The XrmParseCommand function parses an (argc, argv) pair according to the specified option table, loads recognized options into the specified database with
       type “String,” and modifies the (argc, argv) pair to remove all recognized options.  If database contains NULL, XrmParseCommand creates a new database and
       returns a pointer to it.  Otherwise, entries are added to the database specified.  If a database is created, it is created in the current locale.

so when it's all done, the remaining command line has argc=2, and argv[1] is the sysout name which was not recognized by the X option parser.

% lldb ldex
(lldb) target create "ldex"
Current executable set to '/Users/briggs/Projects/maiko/darwin.x86_64/ldex' (x86_64).
(lldb) break set -f main.c -l 342
Breakpoint 1: where = ldex`main + 54 at main.c:342:23, address = 0x000000010004d786
(lldb) run loadups/lisp.sysout -title "FOO" -geometry 100x100+10+10
Process 7246 launched: '/Users/briggs/Projects/maiko/darwin.x86_64/ldex' (x86_64)
Process 7246 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
    frame #0: 0x000000010004d786 ldex`main(argc=6, argv=0x00007ff7bfeff640) at main.c:342:23
   339 	#endif /* MAIKO_ENABLE_FOREIGN_FUNCTION_INTERFACE */
   340 	
   341 	#ifdef XWINDOW
-> 342 	  read_Xoption(&argc, argv);
   343 	#endif /* XWINDOW */
   344 	
   345 	  save_argc = argc;
Target 0: (ldex) stopped.
(lldb) print argc
(int) 6
(lldb) print argv[0]
(char *) 0x00007ff7bfeff7f0 "ldex"
(lldb) print argv[1]
(char *) 0x00007ff7bfeff7f5 "loadups/lisp.sysout"
(lldb) print argv[2]
(char *) 0x00007ff7bfeff809 "-title"
(lldb) print argv[3]
(char *) 0x00007ff7bfeff810 "FOO"
(lldb) print argv[4]
(char *) 0x00007ff7bfeff814 "-geometry"
(lldb) print argv[5]
(char *) 0x00007ff7bfeff81e "100x100+10+10"
(lldb) break set -l 345 
Breakpoint 2: where = ldex`main + 67 at main.c:345:15, address = 0x000000010004d793
(lldb) continue
Process 7246 resuming
Process 7246 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 2.1
    frame #0: 0x000000010004d793 ldex`main(argc=2, argv=0x00007ff7bfeff640) at main.c:345:15
   342 	  read_Xoption(&argc, argv);
   343 	#endif /* XWINDOW */
   344 	
-> 345 	  save_argc = argc;
   346 	  save_argv = argv;
   347 	
   348 	#ifdef PROFILE
Target 0: (ldex) stopped.
(lldb) print argc
(int) 2
(lldb) 

However, it WILL fail if you pass it an option that has not been added to the X options table, because then argc will not be 2 after the read_Xoption(). All the options that lde can take in the ldex case should have been added to the X options table.

[nbriggs]

Actually, the non-X options that can't be specified in the Xresources database (should there be any of those?) need to be accounted for, in the main code, and it shouldn't check for argc==2, but might check that the first remaining argument doesn't start with "-" and use it as the sysout name and then press on to parse any remaining options.

[fghalasz]

Yes, you are right about the failure conditions - there must be one additional argument that is not in the X windows arg table. But then there are lots of args not in the X windows arg table - the nethub stuff for one example. Also args like pixelscale, which apply only to SDL but should just be ignored by X windows.

And we could fix up the immediate bug by just getting rid of the argc = 2 gate (and checking for -).

But, my main problem is that the way its designed is just bad software design for a cross-platform app for a couple of reasons:

1. "Decisions" that apply to all platforms are being duplicated in the code - once for X and once for other platforms. For example, the code that chooses LDESRCSYSOUT, LDESOURCESYSOUT, versus the default of $HOME/lisp.virtualmem if the sysout is not otherwise specified is duplicated in xrdpopt and main. If I want to change that scheme or I wanted, for example, to move the sysout arg to the last argument, then I have to make sure that the code is updated in both (or more) places. And do we really want to duplicate the nethub arg processing in two places?

   And having duplicated code can lead to subtle bugs - for example, if I have no command line sysout, I have LDESRCESYSOUT set to a non-existent file, and I have a viable sysout set in my X resources, lde will still fail because the code in main overrides the code in xrdopt code that chose the X resources sysout.

2. Our scheme of using command line args overrides env variables overrides X resources overrides default values doesn't really work well with the X resources way of doing things unless very carefully managed. We manage to shoehorn it in with the sysout as first arg scheme - but it fails if anyone uses the -sysout arg. And it will fail if we decide to, for example, add a -destsysout arg instead of relying on just passing the dest sysout via LDEDESTSYSOUT.

In the PR that I submitted, I tried to address both of these issues to better rationalize the handling of sysout args.

[nbriggs]

I don't have any complaint about the PR other than the comments should be a bit clearer about what's going on (since you're going making this effort to make the code clearer). We should certainly not encourage people to do things like lde -title foo loadups/lisp.sysout -geometry ... -nh-host ... though it will work in the new case, but the code should have a comment pointing out the behavior of X11 command line parsing - which I had certainly forgotten about until I reviewed it for this bug.