Bug 705895 - X11.so is broken with the default --enable-hidden-visibility
Summary: X11.so is broken with the default --enable-hidden-visibility
Status: CONFIRMED
Alias: None
Product: Ghostscript
Classification: Unclassified
Component: X Display Driver (show other bugs)
Version: 10.0.0
Hardware: PC Linux
: P4 normal
Assignee: Chris Liddell (chrisl)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-09-22 06:35 UTC by Xi Ruoyao
Modified: 2022-09-28 08:04 UTC (History)
2 users (show)

See Also:
Customer:
Word Size: ---


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Xi Ruoyao 2022-09-22 06:35:31 UTC
In the latest 10.0.0 release, -fvisibility=hidden is added into DYNAMIC_CFLAGS.  Unfortunately, X display driver code does not use GSDLLEXPORT, so the symbols defined in X11.so is all hidden now:

$ objdump -T ./obj/X11.so | grep -v UND

./obj/X11.so:     file format elf64-x86-64

DYNAMIC SYMBOL TABLE:

Now the gs executable can't use it.  Using --disable-hidden-visibility can workaround the issue.
Comment 1 Chris Liddell (chrisl) 2022-09-22 15:16:52 UTC
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.
Comment 2 Pierre Labastie 2022-09-22 20:15:55 UTC
(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?
Comment 3 Chris Liddell (chrisl) 2022-09-23 08:05:24 UTC
(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.
Comment 4 Pierre Labastie 2022-09-23 08:09:55 UTC
Ok, thanks for the explanation. Makes sense. The lfs project will use statically linked devices from now on.
Comment 5 Chris Liddell (chrisl) 2022-09-23 08:19:06 UTC
(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.
Comment 6 Pierre Labastie 2022-09-23 08:40:52 UTC
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)
Comment 7 Xi Ruoyao 2022-09-23 08:52:59 UTC
(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.
Comment 8 Xi Ruoyao 2022-09-23 09:45:15 UTC
(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?
Comment 9 Chris Liddell (chrisl) 2022-09-23 10:41:15 UTC
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.
Comment 10 Xi Ruoyao 2022-09-23 10:43:44 UTC
(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!
Comment 11 Chris Liddell (chrisl) 2022-09-23 11:21:26 UTC
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.
Comment 12 Mark Hansen 2022-09-26 10:38:52 UTC
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)
Comment 13 Chris Liddell (chrisl) 2022-09-26 10:56:53 UTC
(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.
Comment 14 Mark Hansen 2022-09-26 11:20:26 UTC
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.
Comment 15 Chris Liddell (chrisl) 2022-09-26 12:57:41 UTC
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
Comment 16 Mark Hansen 2022-09-28 08:04:30 UTC
Alright, thanks, sounds good. Graphviz will just rely on Ghostscript printing the error. Thanks!