Bug 705895

Firstly, I had no idea anyone was actually using that X11 device implementation. Is it really a requirement?

Secondly, --enable-hidden-visibility and --enable-dynamic are, basically, fundamentally incompatible. So the only solution is to either have one disable the other (with a warning), or make attempting to use the two together trigger an error.

The final (preferred) solution would be to deprecate and remove the dynamic devices, since they were only ever intended as a debugging aid, anyway.

(In reply to Chris Liddell (chrisl) from comment #1)
> Firstly, I had no idea anyone was actually using that X11 device
> implementation. Is it really a requirement?
> 
> Secondly, --enable-hidden-visibility and --enable-dynamic are, basically,
> fundamentally incompatible. So the only solution is to either have one
> disable the other (with a warning), or make attempting to use the two
> together trigger an error.
> 
> The final (preferred) solution would be to deprecate and remove the dynamic
> devices, since they were only ever intended as a debugging aid, anyway.

I'm not sure I understand. Do you mean all the x11xxx devices should be removed from ghostscript? I get:
with ghostscript-9.56.1:
$ gs -h | grep x11
Default output device: x11alpha
   tiffscaled8 tiffsep tiffsep1 txtwrite uniprint urf x11 x11alpha x11cmyk
   x11cmyk2 x11cmyk4 x11cmyk8 x11gray2 x11gray4 x11mono xcf xes xpswrite

With ghostscript-10.0.0:
$ gs -h | grep x11
<nothing>

I use x11alpha for displaying postscript images to the screen. Do you think it should be deprecated too?

(In reply to Pierre Labastie from comment #2)
> (In reply to Chris Liddell (chrisl) from comment #1)
> > Firstly, I had no idea anyone was actually using that X11 device
> > implementation. Is it really a requirement?
> > 
> > Secondly, --enable-hidden-visibility and --enable-dynamic are, basically,
> > fundamentally incompatible. So the only solution is to either have one
> > disable the other (with a warning), or make attempting to use the two
> > together trigger an error.
> > 
> > The final (preferred) solution would be to deprecate and remove the dynamic
> > devices, since they were only ever intended as a debugging aid, anyway.
> 
> I'm not sure I understand. Do you mean all the x11xxx devices should be
> removed from ghostscript? I get:
> with ghostscript-9.56.1:
> $ gs -h | grep x11
> Default output device: x11alpha
>    tiffscaled8 tiffsep tiffsep1 txtwrite uniprint urf x11 x11alpha x11cmyk
>    x11cmyk2 x11cmyk4 x11cmyk8 x11gray2 x11gray4 x11mono xcf xes xpswrite
> 
> With ghostscript-10.0.0:
> $ gs -h | grep x11
> <nothing>
> 
> I use x11alpha for displaying postscript images to the screen. Do you think
> it should be deprecated too?


No, there are two forms of X11 device, the "conventional" ones (included if you do not use --enable-dynamic) and these dynamic loading ones (include when --enable-dynamic *is* used).

I'm only talking about deprecating these dynamic loading ones (that rely on the X11.so created by --enable-dynamic).

The conventional X11 ones will *not* be deprecated any time soon, and those are what the *vast* majority of people/distros use.

Ok, thanks for the explanation. Makes sense. The lfs project will use statically linked devices from now on.

(In reply to Pierre Labastie from comment #4)
> Ok, thanks for the explanation. Makes sense. The lfs project will use
> statically linked devices from now on.

If you are happy with that, then it would be very helpful.

I'll leave this open for a few days, just in case you run into any problems.

Well, on our side, I think we can happily go with that. But on your side, I think you need to do something :) When passing --enable-dynamic, a X11.so file is built, that cannot be used (removing it is ok on our side)

(In reply to Chris Liddell (chrisl) from comment #1)
> So the only solution is to either have one
> disable the other (with a warning), or make attempting to use the two
> together trigger an error.

I agree with this.

(In reply to Chris Liddell (chrisl) from comment #5)
> (In reply to Pierre Labastie from comment #4)
> > Ok, thanks for the explanation. Makes sense. The lfs project will use
> > statically linked devices from now on.
> 
> If you are happy with that, then it would be very helpful.
> 
> I'll leave this open for a few days, just in case you run into any problems.

Hi Chris,

We indeed run into a maybe related problem.  dvisvgm fails to build with:

/sources/dvisvgm/dvisvgm-2.14/src/Ghostscript.cpp:382: undefined reference to `gs_error_names'
collect2: error: ld returned 1 exit status

It seems gs_error_names is hidden:

readelf -s soobj/iinit.o | grep gs_error_names 
    55: 0000000000000080   256 OBJECT  GLOBAL HIDDEN     7 gs_error_names

Is it intentional or an oversight?

gs_error_names isn't part of the public API, it's intended to be used only to populate the Postscript error handling dictionary.

I don't have a specific objection to making the symbol visible, in fact, in makes sense for the strings to be visible.

But I'm wary about going too far down that road. Hiding symbols in the .so was in response to a (perfectly valid) complaint, so undoing significant amounts of that is likely to garner complaints again.

(In reply to Chris Liddell (chrisl) from comment #9)
> gs_error_names isn't part of the public API, it's intended to be used only
> to populate the Postscript error handling dictionary.
> 
> I don't have a specific objection to making the symbol visible, in fact, in
> makes sense for the strings to be visible.
> 
> But I'm wary about going too far down that road. Hiding symbols in the .so
> was in response to a (perfectly valid) complaint, so undoing significant
> amounts of that is likely to garner complaints again.

We'll attempt to tell dvisvgm maintainers to fix their code then.  Thanks for the explanation!

If we do want to make those strings "public" (as I think makes some sense), I think the right way to do it would be add an public API call:

int
gsapi_error_string(void *instance, int errcode, char **errstring);

or something like that.

Hi there, I'm a maintainer of Graphviz, I hope this is the right place to report that we've also had reports of dynamic load failures of the `gs_error_names` symbol: https://gitlab.com/graphviz/graphviz/-/issues/2280#note_1113377637

We (Graphviz) were using `gs_error_names` to print GhostScript error messages:

https://gitlab.com/graphviz/graphviz/-/blob/main/plugin/gs/gvloadimage_gs.c#L89:
```
    job->common->errorfn("%s: %s() returned: %d \"%s\" (%s)\n",
		name, funstr, err, gs_error_names[-err - 1], errsrc);
```

I am wondering how we should handle this. Options:

1) We should stop using the symbol: print a raw error number instead. Probably a reasonable short-term fix!
2) Ask for the `gs_error_names` symbol to be made public. Probably not great if you ever want to change the implementation of this symbol.
3) Feature-request some addition to the public API to allow us to get these error messages (so we can help our users with better error messages). Probably some function like Chris mentions above.

(I think it's a pretty good idea that you're locking down your API surface, btw! I'd do the same)

(In reply to Mark Hansen from comment #12)
> Hi there, I'm a maintainer of Graphviz, I hope this is the right place to
> report that we've also had reports of dynamic load failures of the
> `gs_error_names` symbol:
> https://gitlab.com/graphviz/graphviz/-/issues/2280#note_1113377637
> 
> We (Graphviz) were using `gs_error_names` to print GhostScript error
> messages:
> 
> https://gitlab.com/graphviz/graphviz/-/blob/main/plugin/gs/gvloadimage_gs.
> c#L89:
> ```
>     job->common->errorfn("%s: %s() returned: %d \"%s\" (%s)\n",
> 		name, funstr, err, gs_error_names[-err - 1], errsrc);
> ```
> 
> I am wondering how we should handle this. Options:
> 
> 1) We should stop using the symbol: print a raw error number instead.
> Probably a reasonable short-term fix!
> 2) Ask for the `gs_error_names` symbol to be made public. Probably not great
> if you ever want to change the implementation of this symbol.
> 3) Feature-request some addition to the public API to allow us to get these
> error messages (so we can help our users with better error messages).
> Probably some function like Chris mentions above.

My question (genuine question) would be: "Why not just let the Postscript error handling print out the error?"

The fact is, these errors are not a great deal of use for anyone except a Postscript programming initiate, in which case, the entire error message is desirable, rather than just the error name.

Does PostScript print out an error? Is it on stderr? Even when used as library?

If so, Graphviz is probably just being duplicative here (we just write to stderr too), and possibly unhelpful. (I'm a fairly new maintainer of Graphviz, I'm not as familiar with our GS integration, it's very possible we're doing something suboptimal here).

I might imagine some users might want to return a detailed error message over a network response (rather than stderr) but that's not us.

At any rate, I've got a merge-request in to stop Graphviz using this symbol: https://gitlab.com/graphviz/graphviz/-/merge_requests/2856.

Postscript does have its own default error handlers that print errors to the "%stderr" Postscript i/o device. Normally %stderr in Postscript maps to the executable's stderr

Using the gs lib, there are also callback's you can use to "hook" %stdout and %stderr and handle them differently, for example, in a GUI application, documented (sort of) here:
https://ghostscript.readthedocs.io/en/gs10.0.0/API.html#gsapi-set-stdio-with-handle

Alright, thanks, sounds good. Graphviz will just rely on Ghostscript printing the error. Thanks!