Re: Reworked qt terminal is much faster – and fails to work on Mac

[Daniel J S. <dan...@ie...>]

(Ethan, hope you are following below.)

On 02/07/2014 08:01 PM, Mojca Miklavec wrote:
> On Fri, Feb 7, 2014 at 10:03 PM, Thomas Bleher wrote:
>> * Mojca Miklavec [2014-02-07 12:37]:
>>> On Fri, Feb 7, 2014 at 11:50 AM, Mojca Miklavec wrote:
>>>>
>>>> I wanted to test, but it seems that Qt terminal is broken on Mac now.
>>>
>>> The commit that broke gnuplot on Mac seems to be the following one:
>>>
>>> Author: sfeam<sfeam>
>>> Date:   Sat Oct 26 05:44:10 2013 +0000
>>>
>>>      explicit construction and destruction of qt_term data
>>
>> Hmm, this is strange. I have no idea how this patch could have broken
>> the Mac build.
>
> Maybe it just helped a deeper problem surface now. The implementation
> of the Qt terminal never really worked properly on Macs. I might be
> wrong, but I would blame forking. Gnuplot starts two separate Qt
> programs/windows, the proper one and a "fake/nonworking" one. Trying
> to print anything crashes gnuplot (but crash during printing is not
> really a showstopper) etc. On Qt 4 and Mac OS X>= 10.7 one can hide
> the second fake window. On Qt 5 I have no clue how to hide it (so
> users end up with two icons per running gnuplot), but at least
> printing doesn't crash.
>
> It would be great if someone could figure out how to avoid forking
> (and try to avoid it at least on mac) if that is possible at all. I'm
> not skilled enough to understand how gnuplot_qt and forking works to
> be able to change it.

Others have asked for gnuplot to not use fork() for OSX.  I think it is 
ultimately up to someone with an OSX machine to do this.  If we could 
figure out how to do this with threads instead of fork, maybe that is a 
solution.  Fork is a pretty straightforward concept--the existing 
process is duplicated (with a different PID) and in one process the 
"fork()" function returns one value, and in the second process it 
returns a different value.  So, removing some lines of code:

	pid_t pid = fork();
	if (pid < 0)
PARENT		fprintf(stderr, "Forking error\n");
	else if (pid == 0) // Child: start the GUI
	{
CHILD		// Make sure the forked copy doesn't trash the history file
		cancel_history();

		// Start the gnuplot_qt program
		QString filename = getenv("GNUPLOT_DRIVER_DIR");
		if (filename.isEmpty())
			filename = QT_DRIVER_DIR;
		filename += "/gnuplot_qt";

		execlp(filename.toUtf8().data(), "gnuplot_qt", (char*)NULL);
		fprintf(stderr, "Expected Qt driver: %s\n", filename.toUtf8().data());
		perror("Exec failed");
		exit(EXIT_FAILURE);
	}

The parent process will get either PID + or PID -1.  The child process 
gets the PID 0.  For example, there is no way the line labeled PARENT 
will ever happen in the child process because if fork() fails the test 
"if (pid < 0)" will not happen (because there is no such process).  The 
parent process fails the second test ("else if") and continues onward. 
The child process on the other hand passes the test and proceeds to swap 
its process with gnuplot_qt via execlp.

Mac OSX doesn't like fork, or at least it doesn't like how gnuplot is 
doing the fork.  Is there a better way of doing this than fork?  Say 
threads, or spawn?  Before addressing that, let's see if we can make 
fork() work in OSX, because apparently it does have fork()...but the 
code we have might not be "fork-safe".  Here is the best I can find on 
this issue:

http://stackoverflow.com/questions/18854708/why-is-it-prohivited-to-use-fork-without-exec-in-mac

https://mikeash.com/pyblog/friday-qa-2012-01-20-fork-safety.html

The second reference is pretty good.  If I'm understanding correctly, 
gnuplot is supposed to not be doing anything that requires a core OS 
function between the fork() and the exec##() family of functions.  I 
believe the following hunk may be the problem:

		QString filename = getenv("GNUPLOT_DRIVER_DIR");
		if (filename.isEmpty())
			filename = QT_DRIVER_DIR;
		filename += "/gnuplot_qt";

There are a few lines here that are either requesting some system memory 
or are requesting some environment information...either of which could 
fall under core OS function.

I believe this line:

		// Make sure the forked copy doesn't trash the history file
		cancel_history();

is OK because all this routine does is set this variable

static char *expanded_history_filename;

to NULL.  Since it is static it is a memory location within the process 
and not requiring any sort of OS functionality.

SO, with that in mind, Mojca could you try to figure out if a simple mod 
will make this work under OSX?  Try moving that filename hunk of code 
before the fork, say (I'm probably being sloppy and ignoring some more 
direct way of creating a character string from a QString):

// Start the GUI application
void execGnuplotQt()
{
	char *chfilename;

	// Prep the child filename before issuing the fork
	// and avoid any non-fork-safe OS action.
	QString filename = getenv("GNUPLOT_DRIVER_DIR");
	if (filename.isEmpty())
		filename = QT_DRIVER_DIR;
	filename += "/gnuplot_qt";

	chfilename = malloc(filename.length() + 1);
	if (chfilename)
	{
		*chfilename = filename.toUtf8().data();

		// Fork the GUI and exec the gnuplot_qt program in the child process
		pid_t pid = fork();
		if (pid < 0)
			fprintf(stderr, "Forking error\n");
		else if (pid == 0) // Child: start the GUI
		{
			// Make sure the forked copy doesn't trash the history file
			cancel_history();

			// Start the gnuplot_qt program
			execlp(filename.toUtf8().data(), "gnuplot_qt", (char*)NULL);
			fprintf(stderr, "Expected Qt driver: %s\n", filename.toUtf8().data());
			perror("Exec failed");
			exit(EXIT_FAILURE);
		}
	}

	free(chfilename);

	qt_localServerName = "qtgnuplot" + QString::number(pid);
	qt_gnuplot_qtStarted = true;
}

Give that a try.  You may need to do a bit of debugging to make it 
compile.  Let us know if that solves the common Mac OSX all-caps error.

Dan