src/modules/X11/devX11.c, can we remove "#if BUG" yet

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

src/modules/X11/devX11.c, can we remove "#if BUG" yet

frederik
Dear R Devel,

I know that someone put this line in src/modules/X11/devX11.c:2824 for
a reason, because commenting it out causes R to miss an important
ConfigureNotify event in my window manager. The result is that plots
are initially drawn off the window borders, unreadable.

     R_ProcessX11Events((void*) NULL);

Unfortunately for me, this line is commented in the standard release
of R, it has "#if BUG ... #endif" around it.

I guess it is also unfortunate for anyone who uses the same window
manager as I do, namely i3, which I think is pretty popular among Unix
power users these days; not to mention other full-screen window
managers which probably exhibit the same bug in R.

Maybe everyone on the Core team uses twm as their window manager? Or
RStudio on Windows? Which would be sad because then we're not
representing an important user demographic, namely those who prefer
software which is modern and powerful, yet simple to understand and
modify; fully configurable and interoperable and so on.

I first reported this bug 3 years ago. In doing research for my bug
report, I found that the line was commented out by Peter Dalgaard in
2001 with the explanation "X11 segfault fix - I hope".

I don't know what the way forward is. Obviously the Core Team has
reason to say, "look, this isn't very important, it's been broken
since 2001, maybe fixing it will cause the undocumented segfault bug
to reappear, clearly no one here uses your window manager". Do I have
to submit a correctness proof for the proposed change? What do I do?

https://bugs.r-project.org/bugzilla/show_bug.cgi?id=16702

As mentioned in my bug report, I checked using gdb that
ConfigureNotify is indeed being received by the call to
R_ProcessX11Events() when it is uncommented. I haven't experienced any
segfaults.

It's good that Peter left evidence that "R_ProcessX11Events" was being
called 18 years ago from X11DeviceDriver(). If he had deleted the
line, rather than commenting it, then discovering the reason for the
window rendering bug would have been much harder for me.

However, the downside is that now it is not just a matter of inserting
the line where it belongs; I also feel a bit like I have to explain
why it was initially removed. But although I've given it some thought,
I still have no idea.

Somewhat tangentially, I am wondering if there is some way that we
could make the development of R's graphics code proceed at a faster
rate, for example by pulling it out into a separate module, so that
people could offer alternative implementations via CRAN etc., rather
than having R Core be the bottleneck. Would this make sense? Has it
already been done?

Thank you,

Frederick

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: [FORGED] src/modules/X11/devX11.c, can we remove "#if BUG" yet

Paul Murrell-2
Hi

Sorry, I can't offer an explanation for the commented-out line.
However, regarding your final question of avoiding the R-core
bottleneck, you do have the option of creating a third-party graphics
device package.  See, for example, the 'tikzDevice' and 'svglite'
packages on CRAN.  Does that provide you with a way forward ?

Paul

On 20/04/2019 5:27 p.m., [hidden email] wrote:

> Dear R Devel,
>
> I know that someone put this line in src/modules/X11/devX11.c:2824 for
> a reason, because commenting it out causes R to miss an important
> ConfigureNotify event in my window manager. The result is that plots
> are initially drawn off the window borders, unreadable.
>
>     R_ProcessX11Events((void*) NULL);
>
> Unfortunately for me, this line is commented in the standard release
> of R, it has "#if BUG ... #endif" around it.
>
> I guess it is also unfortunate for anyone who uses the same window
> manager as I do, namely i3, which I think is pretty popular among Unix
> power users these days; not to mention other full-screen window
> managers which probably exhibit the same bug in R.
>
> Maybe everyone on the Core team uses twm as their window manager? Or
> RStudio on Windows? Which would be sad because then we're not
> representing an important user demographic, namely those who prefer
> software which is modern and powerful, yet simple to understand and
> modify; fully configurable and interoperable and so on.
>
> I first reported this bug 3 years ago. In doing research for my bug
> report, I found that the line was commented out by Peter Dalgaard in
> 2001 with the explanation "X11 segfault fix - I hope".
>
> I don't know what the way forward is. Obviously the Core Team has
> reason to say, "look, this isn't very important, it's been broken
> since 2001, maybe fixing it will cause the undocumented segfault bug
> to reappear, clearly no one here uses your window manager". Do I have
> to submit a correctness proof for the proposed change? What do I do?
>
> https://bugs.r-project.org/bugzilla/show_bug.cgi?id=16702
>
> As mentioned in my bug report, I checked using gdb that
> ConfigureNotify is indeed being received by the call to
> R_ProcessX11Events() when it is uncommented. I haven't experienced any
> segfaults.
>
> It's good that Peter left evidence that "R_ProcessX11Events" was being
> called 18 years ago from X11DeviceDriver(). If he had deleted the
> line, rather than commenting it, then discovering the reason for the
> window rendering bug would have been much harder for me.
>
> However, the downside is that now it is not just a matter of inserting
> the line where it belongs; I also feel a bit like I have to explain
> why it was initially removed. But although I've given it some thought,
> I still have no idea.
>
> Somewhat tangentially, I am wondering if there is some way that we
> could make the development of R's graphics code proceed at a faster
> rate, for example by pulling it out into a separate module, so that
> people could offer alternative implementations via CRAN etc., rather
> than having R Core be the bottleneck. Would this make sense? Has it
> already been done?
>
> Thank you,
>
> Frederick
>
> ______________________________________________
> [hidden email] mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel

--
Dr Paul Murrell
Department of Statistics
The University of Auckland
Private Bag 92019
Auckland
New Zealand
64 9 3737599 x85392
[hidden email]
http://www.stat.auckland.ac.nz/~paul/

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: [FORGED] src/modules/X11/devX11.c, can we remove "#if BUG" yet

frederik
Thanks Paul for answering the additional question.

I admit that I've only had experience with R's X11 code through work
on a couple of bugs, but for some reason I thought it might be
nontrivial to move it all into a self-contained module due to
interactions with various event loops. The two modules you listed
appear to be used for producing output in image and document formats,
so they don't really cast light on whether this is a problem.

I am not very serious about contributing my time to an effort like
this, but maybe it is good to have some discussion here anyway. I had
thought that maybe authors of alternative plotting interfaces would
have something to say about whether the current graphics design
provides sufficient modularity. Obviously it is modular enough for
RStudio to exist.

Other improvements aside, I think it would just be better to comment
out the old "#if BUG" line, and wait for someone to complain if it
breaks something. A lot has been changed since that line was added, as
I explained in the bug report. I would expect that the bug it was
attempting to fix no longer exists.

Otherwise, what is the next milestone on this bug?

Frederick

On Wed, Apr 24, 2019 at 12:30:44PM +1200, Paul Murrell wrote:

>Hi
>
>Sorry, I can't offer an explanation for the commented-out line.
>However, regarding your final question of avoiding the R-core
>bottleneck, you do have the option of creating a third-party graphics
>device package.  See, for example, the 'tikzDevice' and 'svglite'
>packages on CRAN.  Does that provide you with a way forward ?
>
>Paul
>
>On 20/04/2019 5:27 p.m., [hidden email] wrote:
>>Dear R Devel,
>>
>>I know that someone put this line in src/modules/X11/devX11.c:2824 for
>>a reason, because commenting it out causes R to miss an important
>>ConfigureNotify event in my window manager. The result is that plots
>>are initially drawn off the window borders, unreadable.
>>
>>    R_ProcessX11Events((void*) NULL);
>>
>>Unfortunately for me, this line is commented in the standard release
>>of R, it has "#if BUG ... #endif" around it.
>>
>>I guess it is also unfortunate for anyone who uses the same window
>>manager as I do, namely i3, which I think is pretty popular among Unix
>>power users these days; not to mention other full-screen window
>>managers which probably exhibit the same bug in R.
>>
>>Maybe everyone on the Core team uses twm as their window manager? Or
>>RStudio on Windows? Which would be sad because then we're not
>>representing an important user demographic, namely those who prefer
>>software which is modern and powerful, yet simple to understand and
>>modify; fully configurable and interoperable and so on.
>>
>>I first reported this bug 3 years ago. In doing research for my bug
>>report, I found that the line was commented out by Peter Dalgaard in
>>2001 with the explanation "X11 segfault fix - I hope".
>>
>>I don't know what the way forward is. Obviously the Core Team has
>>reason to say, "look, this isn't very important, it's been broken
>>since 2001, maybe fixing it will cause the undocumented segfault bug
>>to reappear, clearly no one here uses your window manager". Do I have
>>to submit a correctness proof for the proposed change? What do I do?
>>
>>https://bugs.r-project.org/bugzilla/show_bug.cgi?id=16702
>>
>>As mentioned in my bug report, I checked using gdb that
>>ConfigureNotify is indeed being received by the call to
>>R_ProcessX11Events() when it is uncommented. I haven't experienced any
>>segfaults.
>>
>>It's good that Peter left evidence that "R_ProcessX11Events" was being
>>called 18 years ago from X11DeviceDriver(). If he had deleted the
>>line, rather than commenting it, then discovering the reason for the
>>window rendering bug would have been much harder for me.
>>
>>However, the downside is that now it is not just a matter of inserting
>>the line where it belongs; I also feel a bit like I have to explain
>>why it was initially removed. But although I've given it some thought,
>>I still have no idea.
>>
>>Somewhat tangentially, I am wondering if there is some way that we
>>could make the development of R's graphics code proceed at a faster
>>rate, for example by pulling it out into a separate module, so that
>>people could offer alternative implementations via CRAN etc., rather
>>than having R Core be the bottleneck. Would this make sense? Has it
>>already been done?
>>
>>Thank you,
>>
>>Frederick
>>
>>______________________________________________
>>[hidden email] mailing list
>>https://stat.ethz.ch/mailman/listinfo/r-devel
>
>--
>Dr Paul Murrell
>Department of Statistics
>The University of Auckland
>Private Bag 92019
>Auckland
>New Zealand
>64 9 3737599 x85392
>[hidden email]
>http://www.stat.auckland.ac.nz/~paul/
>

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: [FORGED] src/modules/X11/devX11.c, can we remove "#if BUG" yet

Peter Dalgaard-2
In reply to this post by Paul Murrell-2
I don't recall exactly what I did 18 years ago eiher and I likely don't have the time to dig into the archives and reconstruct.

I can imagine that the issue had to do with the protocol around creating and mapping windows. Presumably the segfault comes from looking for events on a window that hasn't been created yet, or has already been destroyed, leading to a NULL reference somewhere. I have a vague recollection that the issue was window manager dependent (in 2001 probably not twm, more likely xvwm on RedHat if it was affecting me).

A proper fix should go via proper understanding of the X11 protocol - uncommenting a line is as bad as commenting it in the 1st place.... So more like "wait for window to exist THEN process events" -- but the 1st part may be WM specific, etc.

I recall docs being quite obtuse, and the X11 "mechanism not policy" credo doesn't help as WMs are not obliged to (say) send notifications, so you can end up stalling, waiting for events that never happen.

It is entirely possible that there is stuff in here that I didn't understand properly at the time, and still don't!

- pd

> On 24 Apr 2019, at 02:30 , Paul Murrell <[hidden email]> wrote:
>
> Hi
>
> Sorry, I can't offer an explanation for the commented-out line.
> However, regarding your final question of avoiding the R-core bottleneck, you do have the option of creating a third-party graphics device package.  See, for example, the 'tikzDevice' and 'svglite' packages on CRAN.  Does that provide you with a way forward ?
>
> Paul
>
> On 20/04/2019 5:27 p.m., [hidden email] wrote:
>> Dear R Devel,
>>
>> I know that someone put this line in src/modules/X11/devX11.c:2824 for
>> a reason, because commenting it out causes R to miss an important
>> ConfigureNotify event in my window manager. The result is that plots
>> are initially drawn off the window borders, unreadable.
>>
>>    R_ProcessX11Events((void*) NULL);
>>
>> Unfortunately for me, this line is commented in the standard release
>> of R, it has "#if BUG ... #endif" around it.
>>
>> I guess it is also unfortunate for anyone who uses the same window
>> manager as I do, namely i3, which I think is pretty popular among Unix
>> power users these days; not to mention other full-screen window
>> managers which probably exhibit the same bug in R.
>>
>> Maybe everyone on the Core team uses twm as their window manager? Or
>> RStudio on Windows? Which would be sad because then we're not
>> representing an important user demographic, namely those who prefer
>> software which is modern and powerful, yet simple to understand and
>> modify; fully configurable and interoperable and so on.
>>
>> I first reported this bug 3 years ago. In doing research for my bug
>> report, I found that the line was commented out by Peter Dalgaard in
>> 2001 with the explanation "X11 segfault fix - I hope".
>>
>> I don't know what the way forward is. Obviously the Core Team has
>> reason to say, "look, this isn't very important, it's been broken
>> since 2001, maybe fixing it will cause the undocumented segfault bug
>> to reappear, clearly no one here uses your window manager". Do I have
>> to submit a correctness proof for the proposed change? What do I do?
>>
>> https://bugs.r-project.org/bugzilla/show_bug.cgi?id=16702
>>
>> As mentioned in my bug report, I checked using gdb that
>> ConfigureNotify is indeed being received by the call to
>> R_ProcessX11Events() when it is uncommented. I haven't experienced any
>> segfaults.
>>
>> It's good that Peter left evidence that "R_ProcessX11Events" was being
>> called 18 years ago from X11DeviceDriver(). If he had deleted the
>> line, rather than commenting it, then discovering the reason for the
>> window rendering bug would have been much harder for me.
>>
>> However, the downside is that now it is not just a matter of inserting
>> the line where it belongs; I also feel a bit like I have to explain
>> why it was initially removed. But although I've given it some thought,
>> I still have no idea.
>>
>> Somewhat tangentially, I am wondering if there is some way that we
>> could make the development of R's graphics code proceed at a faster
>> rate, for example by pulling it out into a separate module, so that
>> people could offer alternative implementations via CRAN etc., rather
>> than having R Core be the bottleneck. Would this make sense? Has it
>> already been done?
>>
>> Thank you,
>>
>> Frederick
>>
>> ______________________________________________
>> [hidden email] mailing list
>> https://stat.ethz.ch/mailman/listinfo/r-devel
>
> --
> Dr Paul Murrell
> Department of Statistics
> The University of Auckland
> Private Bag 92019
> Auckland
> New Zealand
> 64 9 3737599 x85392
> [hidden email]
> http://www.stat.auckland.ac.nz/~paul/
>
> ______________________________________________
> [hidden email] mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel

--
Peter Dalgaard, Professor,
Center for Statistics, Copenhagen Business School
Solbjerg Plads 3, 2000 Frederiksberg, Denmark
Phone: (+45)38153501
Office: A 4.23
Email: [hidden email]  Priv: [hidden email]

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: [FORGED] src/modules/X11/devX11.c, can we remove "#if BUG" yet

Peter Dalgaard-2
OK, so I did the archaeology anyway....


This was the story, R-core November 29, 2001. Part of thread "X11 still segfaults".

------------>>
.....
Gah. I've been too tired today. Why did that take me so long?

The culprit seems to be

R_ProcessEvents((void*) NULL)

in newX11DeviceDriver

This gets called *before* this stuff at the end of Rf_addX11Device

        dd = GEcreateDevDesc(dev);
        addDevice((DevDesc*) dd);
        initDisplayList((DevDesc*) dd);

and it is that "dd" that gets called by Rf_playDisplayList. Removing
the offending line stops the segfaulting, seemingly with no ill
effects.

I'm not really sure what the use of that line ever was; it might be
necessary to make the call somewhere later, but it appears to have
been possible to race past it before receiving any events all the
time.

I also changed a couple of spots missing dd->newDevStruct=1

Will commit in a moment.
<<------------

And the following day, in "graphics saga part III", we had

------------->>
...

I can't make it happen in 1.3.1 but...

It is probably not unrelated to the R_ProcessEvents line that
I took out, but that was definitely wrong. However, one might reenable
it if one could change this bit of code

  if (!(ptr_X11DeviceDriver)((DevDesc*)(dev), display, width, height, ps, gamma,
                                      colormodel, maxcubesize, canvascolor)) {
            free(dev);
            errorcall(gcall, "unable to start device %s", devname);
        }
        gsetVar(install(".Device"), mkString(devname), R_NilValue);
        dd = GEcreateDevDesc(dev);
        addDevice((DevDesc*) dd);
        initDisplayList((DevDesc*) dd);


and put the if-clause last. A cursory clance through the three
functions that are being called didn't reveal anything that would rely
on having opened the device driver first.

Paul?

(I might try it locally, but I'm not sure I should commit anything.)

<<-----------

It seems that the suggestion was never followed up on?

-pd


> On 24 Apr 2019, at 11:42 , peter dalgaard <[hidden email]> wrote:
>
> I don't recall exactly what I did 18 years ago eiher and I likely don't have the time to dig into the archives and reconstruct.
>
> I can imagine that the issue had to do with the protocol around creating and mapping windows. Presumably the segfault comes from looking for events on a window that hasn't been created yet, or has already been destroyed, leading to a NULL reference somewhere. I have a vague recollection that the issue was window manager dependent (in 2001 probably not twm, more likely xvwm on RedHat if it was affecting me).
>
> A proper fix should go via proper understanding of the X11 protocol - uncommenting a line is as bad as commenting it in the 1st place.... So more like "wait for window to exist THEN process events" -- but the 1st part may be WM specific, etc.
>
> I recall docs being quite obtuse, and the X11 "mechanism not policy" credo doesn't help as WMs are not obliged to (say) send notifications, so you can end up stalling, waiting for events that never happen.
>
> It is entirely possible that there is stuff in here that I didn't understand properly at the time, and still don't!
>
> - pd
>
>> On 24 Apr 2019, at 02:30 , Paul Murrell <[hidden email]> wrote:
>>
>> Hi
>>
>> Sorry, I can't offer an explanation for the commented-out line.
>> However, regarding your final question of avoiding the R-core bottleneck, you do have the option of creating a third-party graphics device package.  See, for example, the 'tikzDevice' and 'svglite' packages on CRAN.  Does that provide you with a way forward ?
>>
>> Paul
>>
>> On 20/04/2019 5:27 p.m., [hidden email] wrote:
>>> Dear R Devel,
>>>
>>> I know that someone put this line in src/modules/X11/devX11.c:2824 for
>>> a reason, because commenting it out causes R to miss an important
>>> ConfigureNotify event in my window manager. The result is that plots
>>> are initially drawn off the window borders, unreadable.
>>>
>>>   R_ProcessX11Events((void*) NULL);
>>>
>>> Unfortunately for me, this line is commented in the standard release
>>> of R, it has "#if BUG ... #endif" around it.
>>>
>>> I guess it is also unfortunate for anyone who uses the same window
>>> manager as I do, namely i3, which I think is pretty popular among Unix
>>> power users these days; not to mention other full-screen window
>>> managers which probably exhibit the same bug in R.
>>>
>>> Maybe everyone on the Core team uses twm as their window manager? Or
>>> RStudio on Windows? Which would be sad because then we're not
>>> representing an important user demographic, namely those who prefer
>>> software which is modern and powerful, yet simple to understand and
>>> modify; fully configurable and interoperable and so on.
>>>
>>> I first reported this bug 3 years ago. In doing research for my bug
>>> report, I found that the line was commented out by Peter Dalgaard in
>>> 2001 with the explanation "X11 segfault fix - I hope".
>>>
>>> I don't know what the way forward is. Obviously the Core Team has
>>> reason to say, "look, this isn't very important, it's been broken
>>> since 2001, maybe fixing it will cause the undocumented segfault bug
>>> to reappear, clearly no one here uses your window manager". Do I have
>>> to submit a correctness proof for the proposed change? What do I do?
>>>
>>> https://bugs.r-project.org/bugzilla/show_bug.cgi?id=16702
>>>
>>> As mentioned in my bug report, I checked using gdb that
>>> ConfigureNotify is indeed being received by the call to
>>> R_ProcessX11Events() when it is uncommented. I haven't experienced any
>>> segfaults.
>>>
>>> It's good that Peter left evidence that "R_ProcessX11Events" was being
>>> called 18 years ago from X11DeviceDriver(). If he had deleted the
>>> line, rather than commenting it, then discovering the reason for the
>>> window rendering bug would have been much harder for me.
>>>
>>> However, the downside is that now it is not just a matter of inserting
>>> the line where it belongs; I also feel a bit like I have to explain
>>> why it was initially removed. But although I've given it some thought,
>>> I still have no idea.
>>>
>>> Somewhat tangentially, I am wondering if there is some way that we
>>> could make the development of R's graphics code proceed at a faster
>>> rate, for example by pulling it out into a separate module, so that
>>> people could offer alternative implementations via CRAN etc., rather
>>> than having R Core be the bottleneck. Would this make sense? Has it
>>> already been done?
>>>
>>> Thank you,
>>>
>>> Frederick
>>>
>>> ______________________________________________
>>> [hidden email] mailing list
>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>
>> --
>> Dr Paul Murrell
>> Department of Statistics
>> The University of Auckland
>> Private Bag 92019
>> Auckland
>> New Zealand
>> 64 9 3737599 x85392
>> [hidden email]
>> http://www.stat.auckland.ac.nz/~paul/
>>
>> ______________________________________________
>> [hidden email] mailing list
>> https://stat.ethz.ch/mailman/listinfo/r-devel
>
> --
> Peter Dalgaard, Professor,
> Center for Statistics, Copenhagen Business School
> Solbjerg Plads 3, 2000 Frederiksberg, Denmark
> Phone: (+45)38153501
> Office: A 4.23
> Email: [hidden email]  Priv: [hidden email]
>
>
>
>
>
>
>
>
>

--
Peter Dalgaard, Professor,
Center for Statistics, Copenhagen Business School
Solbjerg Plads 3, 2000 Frederiksberg, Denmark
Phone: (+45)38153501
Office: A 4.23
Email: [hidden email]  Priv: [hidden email]

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: [FORGED] src/modules/X11/devX11.c, can we remove "#if BUG" yet

frederik
Thanks Professor Dalgard.

If you have a different way to fix the bug then I'd be happy to test
it.

Or whatever. I understand that maybe some data was being referenced
before it had been initialized. I could also support moving the
R_ProcessEvents call in another place, but it seems one would also
like to generate some kind of warning message, at the location of the
bad reference, rather than segfaulting. Was it not possible to
identify this location? I'm guessing that Valgrind is a bit more
mature now than it was in 2001...?

Frederick

On Wed, Apr 24, 2019 at 03:12:55PM +0200, peter dalgaard wrote:

>OK, so I did the archaeology anyway....
>
>
>This was the story, R-core November 29, 2001. Part of thread "X11 still segfaults".
>
>------------>>
>.....
>Gah. I've been too tired today. Why did that take me so long?
>
>The culprit seems to be
>
>R_ProcessEvents((void*) NULL)
>
>in newX11DeviceDriver
>
>This gets called *before* this stuff at the end of Rf_addX11Device
>
> dd = GEcreateDevDesc(dev);
> addDevice((DevDesc*) dd);
> initDisplayList((DevDesc*) dd);
>
>and it is that "dd" that gets called by Rf_playDisplayList. Removing
>the offending line stops the segfaulting, seemingly with no ill
>effects.
>
>I'm not really sure what the use of that line ever was; it might be
>necessary to make the call somewhere later, but it appears to have
>been possible to race past it before receiving any events all the
>time.
>
>I also changed a couple of spots missing dd->newDevStruct=1
>
>Will commit in a moment.
><<------------
>
>And the following day, in "graphics saga part III", we had
>
>------------->>
>...
>
>I can't make it happen in 1.3.1 but...
>
>It is probably not unrelated to the R_ProcessEvents line that
>I took out, but that was definitely wrong. However, one might reenable
>it if one could change this bit of code
>
> if (!(ptr_X11DeviceDriver)((DevDesc*)(dev), display, width, height, ps, gamma,
>      colormodel, maxcubesize, canvascolor)) {
>    free(dev);
>    errorcall(gcall, "unable to start device %s", devname);
>       }
> gsetVar(install(".Device"), mkString(devname), R_NilValue);
> dd = GEcreateDevDesc(dev);
> addDevice((DevDesc*) dd);
> initDisplayList((DevDesc*) dd);
>
>
>and put the if-clause last. A cursory clance through the three
>functions that are being called didn't reveal anything that would rely
>on having opened the device driver first.
>
>Paul?
>
>(I might try it locally, but I'm not sure I should commit anything.)
>
><<-----------
>
>It seems that the suggestion was never followed up on?
>
>-pd
>
>
>> On 24 Apr 2019, at 11:42 , peter dalgaard <[hidden email]> wrote:
>>
>> I don't recall exactly what I did 18 years ago eiher and I likely don't have the time to dig into the archives and reconstruct.
>>
>> I can imagine that the issue had to do with the protocol around creating and mapping windows. Presumably the segfault comes from looking for events on a window that hasn't been created yet, or has already been destroyed, leading to a NULL reference somewhere. I have a vague recollection that the issue was window manager dependent (in 2001 probably not twm, more likely xvwm on RedHat if it was affecting me).
>>
>> A proper fix should go via proper understanding of the X11 protocol - uncommenting a line is as bad as commenting it in the 1st place.... So more like "wait for window to exist THEN process events" -- but the 1st part may be WM specific, etc.
>>
>> I recall docs being quite obtuse, and the X11 "mechanism not policy" credo doesn't help as WMs are not obliged to (say) send notifications, so you can end up stalling, waiting for events that never happen.
>>
>> It is entirely possible that there is stuff in here that I didn't understand properly at the time, and still don't!
>>
>> - pd
>>
>>> On 24 Apr 2019, at 02:30 , Paul Murrell <[hidden email]> wrote:
>>>
>>> Hi
>>>
>>> Sorry, I can't offer an explanation for the commented-out line.
>>> However, regarding your final question of avoiding the R-core bottleneck, you do have the option of creating a third-party graphics device package.  See, for example, the 'tikzDevice' and 'svglite' packages on CRAN.  Does that provide you with a way forward ?
>>>
>>> Paul
>>>
>>> On 20/04/2019 5:27 p.m., [hidden email] wrote:
>>>> Dear R Devel,
>>>>
>>>> I know that someone put this line in src/modules/X11/devX11.c:2824 for
>>>> a reason, because commenting it out causes R to miss an important
>>>> ConfigureNotify event in my window manager. The result is that plots
>>>> are initially drawn off the window borders, unreadable.
>>>>
>>>>   R_ProcessX11Events((void*) NULL);
>>>>
>>>> Unfortunately for me, this line is commented in the standard release
>>>> of R, it has "#if BUG ... #endif" around it.
>>>>
>>>> I guess it is also unfortunate for anyone who uses the same window
>>>> manager as I do, namely i3, which I think is pretty popular among Unix
>>>> power users these days; not to mention other full-screen window
>>>> managers which probably exhibit the same bug in R.
>>>>
>>>> Maybe everyone on the Core team uses twm as their window manager? Or
>>>> RStudio on Windows? Which would be sad because then we're not
>>>> representing an important user demographic, namely those who prefer
>>>> software which is modern and powerful, yet simple to understand and
>>>> modify; fully configurable and interoperable and so on.
>>>>
>>>> I first reported this bug 3 years ago. In doing research for my bug
>>>> report, I found that the line was commented out by Peter Dalgaard in
>>>> 2001 with the explanation "X11 segfault fix - I hope".
>>>>
>>>> I don't know what the way forward is. Obviously the Core Team has
>>>> reason to say, "look, this isn't very important, it's been broken
>>>> since 2001, maybe fixing it will cause the undocumented segfault bug
>>>> to reappear, clearly no one here uses your window manager". Do I have
>>>> to submit a correctness proof for the proposed change? What do I do?
>>>>
>>>> https://bugs.r-project.org/bugzilla/show_bug.cgi?id=16702
>>>>
>>>> As mentioned in my bug report, I checked using gdb that
>>>> ConfigureNotify is indeed being received by the call to
>>>> R_ProcessX11Events() when it is uncommented. I haven't experienced any
>>>> segfaults.
>>>>
>>>> It's good that Peter left evidence that "R_ProcessX11Events" was being
>>>> called 18 years ago from X11DeviceDriver(). If he had deleted the
>>>> line, rather than commenting it, then discovering the reason for the
>>>> window rendering bug would have been much harder for me.
>>>>
>>>> However, the downside is that now it is not just a matter of inserting
>>>> the line where it belongs; I also feel a bit like I have to explain
>>>> why it was initially removed. But although I've given it some thought,
>>>> I still have no idea.
>>>>
>>>> Somewhat tangentially, I am wondering if there is some way that we
>>>> could make the development of R's graphics code proceed at a faster
>>>> rate, for example by pulling it out into a separate module, so that
>>>> people could offer alternative implementations via CRAN etc., rather
>>>> than having R Core be the bottleneck. Would this make sense? Has it
>>>> already been done?
>>>>
>>>> Thank you,
>>>>
>>>> Frederick
>>>>
>>>> ______________________________________________
>>>> [hidden email] mailing list
>>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>>
>>> --
>>> Dr Paul Murrell
>>> Department of Statistics
>>> The University of Auckland
>>> Private Bag 92019
>>> Auckland
>>> New Zealand
>>> 64 9 3737599 x85392
>>> [hidden email]
>>> http://www.stat.auckland.ac.nz/~paul/
>>>
>>> ______________________________________________
>>> [hidden email] mailing list
>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>
>> --
>> Peter Dalgaard, Professor,
>> Center for Statistics, Copenhagen Business School
>> Solbjerg Plads 3, 2000 Frederiksberg, Denmark
>> Phone: (+45)38153501
>> Office: A 4.23
>> Email: [hidden email]  Priv: [hidden email]
>>
>>
>>
>>
>>
>>
>>
>>
>>
>
>--
>Peter Dalgaard, Professor,
>Center for Statistics, Copenhagen Business School
>Solbjerg Plads 3, 2000 Frederiksberg, Denmark
>Phone: (+45)38153501
>Office: A 4.23
>Email: [hidden email]  Priv: [hidden email]
>
>
>
>
>
>
>
>
>

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: [FORGED] src/modules/X11/devX11.c, can we remove "#if BUG" yet

Peter Dalgaard-2
I had a look at the current code, and AFAICT it has essentially the same structure as it did back then. I think it may have finally dawned upon me what the issue really is:

The logic is that in Rf_addX11Device, we have

        if (!X11DeviceDriver(dev, display, width, height,
                             ps, gamma, colormodel, maxcubesize,
                             bgcolor, canvascolor, sfonts, res,
                             xpos, ypos, title, useCairo, antialias, family)) {
            free(dev);
            errorcall(call, _("unable to start device %s"), devname);
        }
        dd = GEcreateDevDesc(dev);
        GEaddDevice2(dd, devname);

i.e., we start the device driver, and if it fails, we throw away the "dev" structure and call it a day. If it succeeds, we proceed to create a device descriptor structure and add it to the list of open devices.

This approach means that X11DeviceDriver() cannot do anything that potentially accesses the dd structure because it isn't there yet, and the things it cannot do apparently includes calling R_ProcessX11Events(). [To be completely sure that this is actually still true, I'd need to have a closer look at what handleEvent() does.]

So to fix things, it would seem that you could (a) add the device before attempting to start the driver, preparing to back it out if the driver fails to start, or (b) add a call to R_ProcessX11Events() _after_ the GEaddDevice2(dd, devname). Option (b) is probably the easiest.

Paul: Does this analysis look roughly right?

-pd




> On 26 Apr 2019, at 01:23 , [hidden email] wrote:
>
> Thanks Professor Dalgard.
>
> If you have a different way to fix the bug then I'd be happy to test
> it.
>
> Or whatever. I understand that maybe some data was being referenced
> before it had been initialized. I could also support moving the
> R_ProcessEvents call in another place, but it seems one would also
> like to generate some kind of warning message, at the location of the
> bad reference, rather than segfaulting. Was it not possible to
> identify this location? I'm guessing that Valgrind is a bit more
> mature now than it was in 2001...?
>
> Frederick
>
> On Wed, Apr 24, 2019 at 03:12:55PM +0200, peter dalgaard wrote:
>> OK, so I did the archaeology anyway....
>>
>>
>> This was the story, R-core November 29, 2001. Part of thread "X11 still segfaults".
>>
>> ------------>>
>> .....
>> Gah. I've been too tired today. Why did that take me so long?
>>
>> The culprit seems to be
>>
>> R_ProcessEvents((void*) NULL)
>>
>> in newX11DeviceDriver
>>
>> This gets called *before* this stuff at the end of Rf_addX11Device
>>
>> dd = GEcreateDevDesc(dev);
>> addDevice((DevDesc*) dd);
>> initDisplayList((DevDesc*) dd);
>>
>> and it is that "dd" that gets called by Rf_playDisplayList. Removing
>> the offending line stops the segfaulting, seemingly with no ill
>> effects.
>>
>> I'm not really sure what the use of that line ever was; it might be
>> necessary to make the call somewhere later, but it appears to have
>> been possible to race past it before receiving any events all the
>> time.
>>
>> I also changed a couple of spots missing dd->newDevStruct=1
>>
>> Will commit in a moment.
>> <<------------
>>
>> And the following day, in "graphics saga part III", we had
>>
>> ------------->>
>> ...
>>
>> I can't make it happen in 1.3.1 but...
>>
>> It is probably not unrelated to the R_ProcessEvents line that
>> I took out, but that was definitely wrong. However, one might reenable
>> it if one could change this bit of code
>>
>> if (!(ptr_X11DeviceDriver)((DevDesc*)(dev), display, width, height, ps, gamma,
>>      colormodel, maxcubesize, canvascolor)) {
>>    free(dev);
>>    errorcall(gcall, "unable to start device %s", devname);
>>       }
>> gsetVar(install(".Device"), mkString(devname), R_NilValue);
>> dd = GEcreateDevDesc(dev);
>> addDevice((DevDesc*) dd);
>> initDisplayList((DevDesc*) dd);
>>
>>
>> and put the if-clause last. A cursory clance through the three
>> functions that are being called didn't reveal anything that would rely
>> on having opened the device driver first.
>>
>> Paul?
>>
>> (I might try it locally, but I'm not sure I should commit anything.)
>>
>> <<-----------
>>
>> It seems that the suggestion was never followed up on?
>>
>> -pd
>>
>>
>>> On 24 Apr 2019, at 11:42 , peter dalgaard <[hidden email]> wrote:
>>>
>>> I don't recall exactly what I did 18 years ago eiher and I likely don't have the time to dig into the archives and reconstruct.
>>>
>>> I can imagine that the issue had to do with the protocol around creating and mapping windows. Presumably the segfault comes from looking for events on a window that hasn't been created yet, or has already been destroyed, leading to a NULL reference somewhere. I have a vague recollection that the issue was window manager dependent (in 2001 probably not twm, more likely xvwm on RedHat if it was affecting me).
>>>
>>> A proper fix should go via proper understanding of the X11 protocol - uncommenting a line is as bad as commenting it in the 1st place.... So more like "wait for window to exist THEN process events" -- but the 1st part may be WM specific, etc.
>>>
>>> I recall docs being quite obtuse, and the X11 "mechanism not policy" credo doesn't help as WMs are not obliged to (say) send notifications, so you can end up stalling, waiting for events that never happen.
>>>
>>> It is entirely possible that there is stuff in here that I didn't understand properly at the time, and still don't!
>>>
>>> - pd
>>>
>>>> On 24 Apr 2019, at 02:30 , Paul Murrell <[hidden email]> wrote:
>>>>
>>>> Hi
>>>>
>>>> Sorry, I can't offer an explanation for the commented-out line.
>>>> However, regarding your final question of avoiding the R-core bottleneck, you do have the option of creating a third-party graphics device package.  See, for example, the 'tikzDevice' and 'svglite' packages on CRAN.  Does that provide you with a way forward ?
>>>>
>>>> Paul
>>>>
>>>> On 20/04/2019 5:27 p.m., [hidden email] wrote:
>>>>> Dear R Devel,
>>>>>
>>>>> I know that someone put this line in src/modules/X11/devX11.c:2824 for
>>>>> a reason, because commenting it out causes R to miss an important
>>>>> ConfigureNotify event in my window manager. The result is that plots
>>>>> are initially drawn off the window borders, unreadable.
>>>>>
>>>>>  R_ProcessX11Events((void*) NULL);
>>>>>
>>>>> Unfortunately for me, this line is commented in the standard release
>>>>> of R, it has "#if BUG ... #endif" around it.
>>>>>
>>>>> I guess it is also unfortunate for anyone who uses the same window
>>>>> manager as I do, namely i3, which I think is pretty popular among Unix
>>>>> power users these days; not to mention other full-screen window
>>>>> managers which probably exhibit the same bug in R.
>>>>>
>>>>> Maybe everyone on the Core team uses twm as their window manager? Or
>>>>> RStudio on Windows? Which would be sad because then we're not
>>>>> representing an important user demographic, namely those who prefer
>>>>> software which is modern and powerful, yet simple to understand and
>>>>> modify; fully configurable and interoperable and so on.
>>>>>
>>>>> I first reported this bug 3 years ago. In doing research for my bug
>>>>> report, I found that the line was commented out by Peter Dalgaard in
>>>>> 2001 with the explanation "X11 segfault fix - I hope".
>>>>>
>>>>> I don't know what the way forward is. Obviously the Core Team has
>>>>> reason to say, "look, this isn't very important, it's been broken
>>>>> since 2001, maybe fixing it will cause the undocumented segfault bug
>>>>> to reappear, clearly no one here uses your window manager". Do I have
>>>>> to submit a correctness proof for the proposed change? What do I do?
>>>>>
>>>>> https://bugs.r-project.org/bugzilla/show_bug.cgi?id=16702
>>>>>
>>>>> As mentioned in my bug report, I checked using gdb that
>>>>> ConfigureNotify is indeed being received by the call to
>>>>> R_ProcessX11Events() when it is uncommented. I haven't experienced any
>>>>> segfaults.
>>>>>
>>>>> It's good that Peter left evidence that "R_ProcessX11Events" was being
>>>>> called 18 years ago from X11DeviceDriver(). If he had deleted the
>>>>> line, rather than commenting it, then discovering the reason for the
>>>>> window rendering bug would have been much harder for me.
>>>>>
>>>>> However, the downside is that now it is not just a matter of inserting
>>>>> the line where it belongs; I also feel a bit like I have to explain
>>>>> why it was initially removed. But although I've given it some thought,
>>>>> I still have no idea.
>>>>>
>>>>> Somewhat tangentially, I am wondering if there is some way that we
>>>>> could make the development of R's graphics code proceed at a faster
>>>>> rate, for example by pulling it out into a separate module, so that
>>>>> people could offer alternative implementations via CRAN etc., rather
>>>>> than having R Core be the bottleneck. Would this make sense? Has it
>>>>> already been done?
>>>>>
>>>>> Thank you,
>>>>>
>>>>> Frederick
>>>>>
>>>>> ______________________________________________
>>>>> [hidden email] mailing list
>>>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>
>>>> --
>>>> Dr Paul Murrell
>>>> Department of Statistics
>>>> The University of Auckland
>>>> Private Bag 92019
>>>> Auckland
>>>> New Zealand
>>>> 64 9 3737599 x85392
>>>> [hidden email]
>>>> http://www.stat.auckland.ac.nz/~paul/
>>>>
>>>> ______________________________________________
>>>> [hidden email] mailing list
>>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>>
>>> --
>>> Peter Dalgaard, Professor,
>>> Center for Statistics, Copenhagen Business School
>>> Solbjerg Plads 3, 2000 Frederiksberg, Denmark
>>> Phone: (+45)38153501
>>> Office: A 4.23
>>> Email: [hidden email]  Priv: [hidden email]
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>> --
>> Peter Dalgaard, Professor,
>> Center for Statistics, Copenhagen Business School
>> Solbjerg Plads 3, 2000 Frederiksberg, Denmark
>> Phone: (+45)38153501
>> Office: A 4.23
>> Email: [hidden email]  Priv: [hidden email]
>>
>>
>>
>>
>>
>>
>>
>>
>>

--
Peter Dalgaard, Professor,
Center for Statistics, Copenhagen Business School
Solbjerg Plads 3, 2000 Frederiksberg, Denmark
Phone: (+45)38153501
Office: A 4.23
Email: [hidden email]  Priv: [hidden email]

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: [FORGED] src/modules/X11/devX11.c, can we remove "#if BUG" yet

Paul Murrell-2
Hi Peter

Yes, that looks roughly right to me.  I would be in favour of your
option (b), partly because it is probably easiest and partly because
that retains the basic graphics device startup logic pattern that is
replicated across all(?) graphics devices.

Paul

On 28/04/19 11:39 AM, peter dalgaard wrote:

> I had a look at the current code, and AFAICT it has essentially the same structure as it did back then. I think it may have finally dawned upon me what the issue really is:
>
> The logic is that in Rf_addX11Device, we have
>
>          if (!X11DeviceDriver(dev, display, width, height,
>                               ps, gamma, colormodel, maxcubesize,
>                               bgcolor, canvascolor, sfonts, res,
>                               xpos, ypos, title, useCairo, antialias, family)) {
>              free(dev);
>              errorcall(call, _("unable to start device %s"), devname);
>          }
>          dd = GEcreateDevDesc(dev);
>          GEaddDevice2(dd, devname);
>
> i.e., we start the device driver, and if it fails, we throw away the "dev" structure and call it a day. If it succeeds, we proceed to create a device descriptor structure and add it to the list of open devices.
>
> This approach means that X11DeviceDriver() cannot do anything that potentially accesses the dd structure because it isn't there yet, and the things it cannot do apparently includes calling R_ProcessX11Events(). [To be completely sure that this is actually still true, I'd need to have a closer look at what handleEvent() does.]
>
> So to fix things, it would seem that you could (a) add the device before attempting to start the driver, preparing to back it out if the driver fails to start, or (b) add a call to R_ProcessX11Events() _after_ the GEaddDevice2(dd, devname). Option (b) is probably the easiest.
>
> Paul: Does this analysis look roughly right?
>
> -pd
>
>
>
>
>> On 26 Apr 2019, at 01:23 , [hidden email] wrote:
>>
>> Thanks Professor Dalgard.
>>
>> If you have a different way to fix the bug then I'd be happy to test
>> it.
>>
>> Or whatever. I understand that maybe some data was being referenced
>> before it had been initialized. I could also support moving the
>> R_ProcessEvents call in another place, but it seems one would also
>> like to generate some kind of warning message, at the location of the
>> bad reference, rather than segfaulting. Was it not possible to
>> identify this location? I'm guessing that Valgrind is a bit more
>> mature now than it was in 2001...?
>>
>> Frederick
>>
>> On Wed, Apr 24, 2019 at 03:12:55PM +0200, peter dalgaard wrote:
>>> OK, so I did the archaeology anyway....
>>>
>>>
>>> This was the story, R-core November 29, 2001. Part of thread "X11 still segfaults".
>>>
>>> ------------>>
>>> .....
>>> Gah. I've been too tired today. Why did that take me so long?
>>>
>>> The culprit seems to be
>>>
>>> R_ProcessEvents((void*) NULL)
>>>
>>> in newX11DeviceDriver
>>>
>>> This gets called *before* this stuff at the end of Rf_addX11Device
>>>
>>> dd = GEcreateDevDesc(dev);
>>> addDevice((DevDesc*) dd);
>>> initDisplayList((DevDesc*) dd);
>>>
>>> and it is that "dd" that gets called by Rf_playDisplayList. Removing
>>> the offending line stops the segfaulting, seemingly with no ill
>>> effects.
>>>
>>> I'm not really sure what the use of that line ever was; it might be
>>> necessary to make the call somewhere later, but it appears to have
>>> been possible to race past it before receiving any events all the
>>> time.
>>>
>>> I also changed a couple of spots missing dd->newDevStruct=1
>>>
>>> Will commit in a moment.
>>> <<------------
>>>
>>> And the following day, in "graphics saga part III", we had
>>>
>>> ------------->>
>>> ...
>>>
>>> I can't make it happen in 1.3.1 but...
>>>
>>> It is probably not unrelated to the R_ProcessEvents line that
>>> I took out, but that was definitely wrong. However, one might reenable
>>> it if one could change this bit of code
>>>
>>> if (!(ptr_X11DeviceDriver)((DevDesc*)(dev), display, width, height, ps, gamma,
>>>      colormodel, maxcubesize, canvascolor)) {
>>>    free(dev);
>>>    errorcall(gcall, "unable to start device %s", devname);
>>>       }
>>> gsetVar(install(".Device"), mkString(devname), R_NilValue);
>>> dd = GEcreateDevDesc(dev);
>>> addDevice((DevDesc*) dd);
>>> initDisplayList((DevDesc*) dd);
>>>
>>>
>>> and put the if-clause last. A cursory clance through the three
>>> functions that are being called didn't reveal anything that would rely
>>> on having opened the device driver first.
>>>
>>> Paul?
>>>
>>> (I might try it locally, but I'm not sure I should commit anything.)
>>>
>>> <<-----------
>>>
>>> It seems that the suggestion was never followed up on?
>>>
>>> -pd
>>>
>>>
>>>> On 24 Apr 2019, at 11:42 , peter dalgaard <[hidden email]> wrote:
>>>>
>>>> I don't recall exactly what I did 18 years ago eiher and I likely don't have the time to dig into the archives and reconstruct.
>>>>
>>>> I can imagine that the issue had to do with the protocol around creating and mapping windows. Presumably the segfault comes from looking for events on a window that hasn't been created yet, or has already been destroyed, leading to a NULL reference somewhere. I have a vague recollection that the issue was window manager dependent (in 2001 probably not twm, more likely xvwm on RedHat if it was affecting me).
>>>>
>>>> A proper fix should go via proper understanding of the X11 protocol - uncommenting a line is as bad as commenting it in the 1st place.... So more like "wait for window to exist THEN process events" -- but the 1st part may be WM specific, etc.
>>>>
>>>> I recall docs being quite obtuse, and the X11 "mechanism not policy" credo doesn't help as WMs are not obliged to (say) send notifications, so you can end up stalling, waiting for events that never happen.
>>>>
>>>> It is entirely possible that there is stuff in here that I didn't understand properly at the time, and still don't!
>>>>
>>>> - pd
>>>>
>>>>> On 24 Apr 2019, at 02:30 , Paul Murrell <[hidden email]> wrote:
>>>>>
>>>>> Hi
>>>>>
>>>>> Sorry, I can't offer an explanation for the commented-out line.
>>>>> However, regarding your final question of avoiding the R-core bottleneck, you do have the option of creating a third-party graphics device package.  See, for example, the 'tikzDevice' and 'svglite' packages on CRAN.  Does that provide you with a way forward ?
>>>>>
>>>>> Paul
>>>>>
>>>>> On 20/04/2019 5:27 p.m., [hidden email] wrote:
>>>>>> Dear R Devel,
>>>>>>
>>>>>> I know that someone put this line in src/modules/X11/devX11.c:2824 for
>>>>>> a reason, because commenting it out causes R to miss an important
>>>>>> ConfigureNotify event in my window manager. The result is that plots
>>>>>> are initially drawn off the window borders, unreadable.
>>>>>>
>>>>>>   R_ProcessX11Events((void*) NULL);
>>>>>>
>>>>>> Unfortunately for me, this line is commented in the standard release
>>>>>> of R, it has "#if BUG ... #endif" around it.
>>>>>>
>>>>>> I guess it is also unfortunate for anyone who uses the same window
>>>>>> manager as I do, namely i3, which I think is pretty popular among Unix
>>>>>> power users these days; not to mention other full-screen window
>>>>>> managers which probably exhibit the same bug in R.
>>>>>>
>>>>>> Maybe everyone on the Core team uses twm as their window manager? Or
>>>>>> RStudio on Windows? Which would be sad because then we're not
>>>>>> representing an important user demographic, namely those who prefer
>>>>>> software which is modern and powerful, yet simple to understand and
>>>>>> modify; fully configurable and interoperable and so on.
>>>>>>
>>>>>> I first reported this bug 3 years ago. In doing research for my bug
>>>>>> report, I found that the line was commented out by Peter Dalgaard in
>>>>>> 2001 with the explanation "X11 segfault fix - I hope".
>>>>>>
>>>>>> I don't know what the way forward is. Obviously the Core Team has
>>>>>> reason to say, "look, this isn't very important, it's been broken
>>>>>> since 2001, maybe fixing it will cause the undocumented segfault bug
>>>>>> to reappear, clearly no one here uses your window manager". Do I have
>>>>>> to submit a correctness proof for the proposed change? What do I do?
>>>>>>
>>>>>> https://bugs.r-project.org/bugzilla/show_bug.cgi?id=16702
>>>>>>
>>>>>> As mentioned in my bug report, I checked using gdb that
>>>>>> ConfigureNotify is indeed being received by the call to
>>>>>> R_ProcessX11Events() when it is uncommented. I haven't experienced any
>>>>>> segfaults.
>>>>>>
>>>>>> It's good that Peter left evidence that "R_ProcessX11Events" was being
>>>>>> called 18 years ago from X11DeviceDriver(). If he had deleted the
>>>>>> line, rather than commenting it, then discovering the reason for the
>>>>>> window rendering bug would have been much harder for me.
>>>>>>
>>>>>> However, the downside is that now it is not just a matter of inserting
>>>>>> the line where it belongs; I also feel a bit like I have to explain
>>>>>> why it was initially removed. But although I've given it some thought,
>>>>>> I still have no idea.
>>>>>>
>>>>>> Somewhat tangentially, I am wondering if there is some way that we
>>>>>> could make the development of R's graphics code proceed at a faster
>>>>>> rate, for example by pulling it out into a separate module, so that
>>>>>> people could offer alternative implementations via CRAN etc., rather
>>>>>> than having R Core be the bottleneck. Would this make sense? Has it
>>>>>> already been done?
>>>>>>
>>>>>> Thank you,
>>>>>>
>>>>>> Frederick
>>>>>>
>>>>>> ______________________________________________
>>>>>> [hidden email] mailing list
>>>>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>>
>>>>> --
>>>>> Dr Paul Murrell
>>>>> Department of Statistics
>>>>> The University of Auckland
>>>>> Private Bag 92019
>>>>> Auckland
>>>>> New Zealand
>>>>> 64 9 3737599 x85392
>>>>> [hidden email]
>>>>> http://www.stat.auckland.ac.nz/~paul/
>>>>>
>>>>> ______________________________________________
>>>>> [hidden email] mailing list
>>>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>
>>>> --
>>>> Peter Dalgaard, Professor,
>>>> Center for Statistics, Copenhagen Business School
>>>> Solbjerg Plads 3, 2000 Frederiksberg, Denmark
>>>> Phone: (+45)38153501
>>>> Office: A 4.23
>>>> Email: [hidden email]  Priv: [hidden email]
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>> --
>>> Peter Dalgaard, Professor,
>>> Center for Statistics, Copenhagen Business School
>>> Solbjerg Plads 3, 2000 Frederiksberg, Denmark
>>> Phone: (+45)38153501
>>> Office: A 4.23
>>> Email: [hidden email]  Priv: [hidden email]
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>

--
Dr Paul Murrell
Department of Statistics
The University of Auckland
Private Bag 92019
Auckland
New Zealand
64 9 3737599 x85392
[hidden email]
http://www.stat.auckland.ac.nz/~paul/

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: [FORGED] src/modules/X11/devX11.c, can we remove "#if BUG" yet

Peter Dalgaard-2
OK, this is now in R-devel, but only superficially tested (b/c this is a Mac). Please check it out.

-pd

> On 30 Apr 2019, at 23:09 , Paul Murrell <[hidden email]> wrote:
>
> Hi Peter
>
> Yes, that looks roughly right to me.  I would be in favour of your option (b), partly because it is probably easiest and partly because that retains the basic graphics device startup logic pattern that is replicated across all(?) graphics devices.
>
> Paul
>
> On 28/04/19 11:39 AM, peter dalgaard wrote:
>> I had a look at the current code, and AFAICT it has essentially the same structure as it did back then. I think it may have finally dawned upon me what the issue really is:
>> The logic is that in Rf_addX11Device, we have
>>         if (!X11DeviceDriver(dev, display, width, height,
>>                              ps, gamma, colormodel, maxcubesize,
>>                              bgcolor, canvascolor, sfonts, res,
>>                              xpos, ypos, title, useCairo, antialias, family)) {
>>             free(dev);
>>             errorcall(call, _("unable to start device %s"), devname);
>>         }
>>         dd = GEcreateDevDesc(dev);
>>         GEaddDevice2(dd, devname);
>> i.e., we start the device driver, and if it fails, we throw away the "dev" structure and call it a day. If it succeeds, we proceed to create a device descriptor structure and add it to the list of open devices.
>> This approach means that X11DeviceDriver() cannot do anything that potentially accesses the dd structure because it isn't there yet, and the things it cannot do apparently includes calling R_ProcessX11Events(). [To be completely sure that this is actually still true, I'd need to have a closer look at what handleEvent() does.]
>> So to fix things, it would seem that you could (a) add the device before attempting to start the driver, preparing to back it out if the driver fails to start, or (b) add a call to R_ProcessX11Events() _after_ the GEaddDevice2(dd, devname). Option (b) is probably the easiest.
>> Paul: Does this analysis look roughly right?
>> -pd
>>> On 26 Apr 2019, at 01:23 , [hidden email] wrote:
>>>
>>> Thanks Professor Dalgard.
>>>
>>> If you have a different way to fix the bug then I'd be happy to test
>>> it.
>>>
>>> Or whatever. I understand that maybe some data was being referenced
>>> before it had been initialized. I could also support moving the
>>> R_ProcessEvents call in another place, but it seems one would also
>>> like to generate some kind of warning message, at the location of the
>>> bad reference, rather than segfaulting. Was it not possible to
>>> identify this location? I'm guessing that Valgrind is a bit more
>>> mature now than it was in 2001...?
>>>
>>> Frederick
>>>
>>> On Wed, Apr 24, 2019 at 03:12:55PM +0200, peter dalgaard wrote:
>>>> OK, so I did the archaeology anyway....
>>>>
>>>>
>>>> This was the story, R-core November 29, 2001. Part of thread "X11 still segfaults".
>>>>
>>>> ------------>>
>>>> .....
>>>> Gah. I've been too tired today. Why did that take me so long?
>>>>
>>>> The culprit seems to be
>>>>
>>>> R_ProcessEvents((void*) NULL)
>>>>
>>>> in newX11DeviceDriver
>>>>
>>>> This gets called *before* this stuff at the end of Rf_addX11Device
>>>>
>>>> dd = GEcreateDevDesc(dev);
>>>> addDevice((DevDesc*) dd);
>>>> initDisplayList((DevDesc*) dd);
>>>>
>>>> and it is that "dd" that gets called by Rf_playDisplayList. Removing
>>>> the offending line stops the segfaulting, seemingly with no ill
>>>> effects.
>>>>
>>>> I'm not really sure what the use of that line ever was; it might be
>>>> necessary to make the call somewhere later, but it appears to have
>>>> been possible to race past it before receiving any events all the
>>>> time.
>>>>
>>>> I also changed a couple of spots missing dd->newDevStruct=1
>>>>
>>>> Will commit in a moment.
>>>> <<------------
>>>>
>>>> And the following day, in "graphics saga part III", we had
>>>>
>>>> ------------->>
>>>> ...
>>>>
>>>> I can't make it happen in 1.3.1 but...
>>>>
>>>> It is probably not unrelated to the R_ProcessEvents line that
>>>> I took out, but that was definitely wrong. However, one might reenable
>>>> it if one could change this bit of code
>>>>
>>>> if (!(ptr_X11DeviceDriver)((DevDesc*)(dev), display, width, height, ps, gamma,
>>>>      colormodel, maxcubesize, canvascolor)) {
>>>>    free(dev);
>>>>    errorcall(gcall, "unable to start device %s", devname);
>>>>       }
>>>> gsetVar(install(".Device"), mkString(devname), R_NilValue);
>>>> dd = GEcreateDevDesc(dev);
>>>> addDevice((DevDesc*) dd);
>>>> initDisplayList((DevDesc*) dd);
>>>>
>>>>
>>>> and put the if-clause last. A cursory clance through the three
>>>> functions that are being called didn't reveal anything that would rely
>>>> on having opened the device driver first.
>>>>
>>>> Paul?
>>>>
>>>> (I might try it locally, but I'm not sure I should commit anything.)
>>>>
>>>> <<-----------
>>>>
>>>> It seems that the suggestion was never followed up on?
>>>>
>>>> -pd
>>>>
>>>>
>>>>> On 24 Apr 2019, at 11:42 , peter dalgaard <[hidden email]> wrote:
>>>>>
>>>>> I don't recall exactly what I did 18 years ago eiher and I likely don't have the time to dig into the archives and reconstruct.
>>>>>
>>>>> I can imagine that the issue had to do with the protocol around creating and mapping windows. Presumably the segfault comes from looking for events on a window that hasn't been created yet, or has already been destroyed, leading to a NULL reference somewhere. I have a vague recollection that the issue was window manager dependent (in 2001 probably not twm, more likely xvwm on RedHat if it was affecting me).
>>>>>
>>>>> A proper fix should go via proper understanding of the X11 protocol - uncommenting a line is as bad as commenting it in the 1st place.... So more like "wait for window to exist THEN process events" -- but the 1st part may be WM specific, etc.
>>>>>
>>>>> I recall docs being quite obtuse, and the X11 "mechanism not policy" credo doesn't help as WMs are not obliged to (say) send notifications, so you can end up stalling, waiting for events that never happen.
>>>>>
>>>>> It is entirely possible that there is stuff in here that I didn't understand properly at the time, and still don't!
>>>>>
>>>>> - pd
>>>>>
>>>>>> On 24 Apr 2019, at 02:30 , Paul Murrell <[hidden email]> wrote:
>>>>>>
>>>>>> Hi
>>>>>>
>>>>>> Sorry, I can't offer an explanation for the commented-out line.
>>>>>> However, regarding your final question of avoiding the R-core bottleneck, you do have the option of creating a third-party graphics device package.  See, for example, the 'tikzDevice' and 'svglite' packages on CRAN.  Does that provide you with a way forward ?
>>>>>>
>>>>>> Paul
>>>>>>
>>>>>> On 20/04/2019 5:27 p.m., [hidden email] wrote:
>>>>>>> Dear R Devel,
>>>>>>>
>>>>>>> I know that someone put this line in src/modules/X11/devX11.c:2824 for
>>>>>>> a reason, because commenting it out causes R to miss an important
>>>>>>> ConfigureNotify event in my window manager. The result is that plots
>>>>>>> are initially drawn off the window borders, unreadable.
>>>>>>>
>>>>>>>  R_ProcessX11Events((void*) NULL);
>>>>>>>
>>>>>>> Unfortunately for me, this line is commented in the standard release
>>>>>>> of R, it has "#if BUG ... #endif" around it.
>>>>>>>
>>>>>>> I guess it is also unfortunate for anyone who uses the same window
>>>>>>> manager as I do, namely i3, which I think is pretty popular among Unix
>>>>>>> power users these days; not to mention other full-screen window
>>>>>>> managers which probably exhibit the same bug in R.
>>>>>>>
>>>>>>> Maybe everyone on the Core team uses twm as their window manager? Or
>>>>>>> RStudio on Windows? Which would be sad because then we're not
>>>>>>> representing an important user demographic, namely those who prefer
>>>>>>> software which is modern and powerful, yet simple to understand and
>>>>>>> modify; fully configurable and interoperable and so on.
>>>>>>>
>>>>>>> I first reported this bug 3 years ago. In doing research for my bug
>>>>>>> report, I found that the line was commented out by Peter Dalgaard in
>>>>>>> 2001 with the explanation "X11 segfault fix - I hope".
>>>>>>>
>>>>>>> I don't know what the way forward is. Obviously the Core Team has
>>>>>>> reason to say, "look, this isn't very important, it's been broken
>>>>>>> since 2001, maybe fixing it will cause the undocumented segfault bug
>>>>>>> to reappear, clearly no one here uses your window manager". Do I have
>>>>>>> to submit a correctness proof for the proposed change? What do I do?
>>>>>>>
>>>>>>> https://bugs.r-project.org/bugzilla/show_bug.cgi?id=16702
>>>>>>>
>>>>>>> As mentioned in my bug report, I checked using gdb that
>>>>>>> ConfigureNotify is indeed being received by the call to
>>>>>>> R_ProcessX11Events() when it is uncommented. I haven't experienced any
>>>>>>> segfaults.
>>>>>>>
>>>>>>> It's good that Peter left evidence that "R_ProcessX11Events" was being
>>>>>>> called 18 years ago from X11DeviceDriver(). If he had deleted the
>>>>>>> line, rather than commenting it, then discovering the reason for the
>>>>>>> window rendering bug would have been much harder for me.
>>>>>>>
>>>>>>> However, the downside is that now it is not just a matter of inserting
>>>>>>> the line where it belongs; I also feel a bit like I have to explain
>>>>>>> why it was initially removed. But although I've given it some thought,
>>>>>>> I still have no idea.
>>>>>>>
>>>>>>> Somewhat tangentially, I am wondering if there is some way that we
>>>>>>> could make the development of R's graphics code proceed at a faster
>>>>>>> rate, for example by pulling it out into a separate module, so that
>>>>>>> people could offer alternative implementations via CRAN etc., rather
>>>>>>> than having R Core be the bottleneck. Would this make sense? Has it
>>>>>>> already been done?
>>>>>>>
>>>>>>> Thank you,
>>>>>>>
>>>>>>> Frederick
>>>>>>>
>>>>>>> ______________________________________________
>>>>>>> [hidden email] mailing list
>>>>>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>>>
>>>>>> --
>>>>>> Dr Paul Murrell
>>>>>> Department of Statistics
>>>>>> The University of Auckland
>>>>>> Private Bag 92019
>>>>>> Auckland
>>>>>> New Zealand
>>>>>> 64 9 3737599 x85392
>>>>>> [hidden email]
>>>>>> http://www.stat.auckland.ac.nz/~paul/
>>>>>>
>>>>>> ______________________________________________
>>>>>> [hidden email] mailing list
>>>>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>>
>>>>> --
>>>>> Peter Dalgaard, Professor,
>>>>> Center for Statistics, Copenhagen Business School
>>>>> Solbjerg Plads 3, 2000 Frederiksberg, Denmark
>>>>> Phone: (+45)38153501
>>>>> Office: A 4.23
>>>>> Email: [hidden email]  Priv: [hidden email]
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>> --
>>>> Peter Dalgaard, Professor,
>>>> Center for Statistics, Copenhagen Business School
>>>> Solbjerg Plads 3, 2000 Frederiksberg, Denmark
>>>> Phone: (+45)38153501
>>>> Office: A 4.23
>>>> Email: [hidden email]  Priv: [hidden email]
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>
> --
> Dr Paul Murrell
> Department of Statistics
> The University of Auckland
> Private Bag 92019
> Auckland
> New Zealand
> 64 9 3737599 x85392
> [hidden email]
> http://www.stat.auckland.ac.nz/~paul/

--
Peter Dalgaard, Professor,
Center for Statistics, Copenhagen Business School
Solbjerg Plads 3, 2000 Frederiksberg, Denmark
Phone: (+45)38153501
Office: A 4.23
Email: [hidden email]  Priv: [hidden email]

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: [FORGED] src/modules/X11/devX11.c, can we remove "#if BUG" yet

frederik
I tested it. It fixes the bug and didn't seem to produce any errors. Thank you Professor Dalgaard! I'm so glad this has finally been addressed. I will update the bug report.

(https://bugs.r-project.org/bugzilla/show_bug.cgi?id=16702)

On Thu, May 02, 2019 at 04:48:51PM +0200, peter dalgaard wrote:

>OK, this is now in R-devel, but only superficially tested (b/c this is a Mac). Please check it out.
>
>-pd
>
>> On 30 Apr 2019, at 23:09 , Paul Murrell <[hidden email]> wrote:
>>
>> Hi Peter
>>
>> Yes, that looks roughly right to me.  I would be in favour of your option (b), partly because it is probably easiest and partly because that retains the basic graphics device startup logic pattern that is replicated across all(?) graphics devices.
>>
>> Paul
>>
>> On 28/04/19 11:39 AM, peter dalgaard wrote:
>>> I had a look at the current code, and AFAICT it has essentially the same structure as it did back then. I think it may have finally dawned upon me what the issue really is:
>>> The logic is that in Rf_addX11Device, we have
>>>         if (!X11DeviceDriver(dev, display, width, height,
>>>                              ps, gamma, colormodel, maxcubesize,
>>>                              bgcolor, canvascolor, sfonts, res,
>>>                              xpos, ypos, title, useCairo, antialias, family)) {
>>>             free(dev);
>>>             errorcall(call, _("unable to start device %s"), devname);
>>>         }
>>>         dd = GEcreateDevDesc(dev);
>>>         GEaddDevice2(dd, devname);
>>> i.e., we start the device driver, and if it fails, we throw away the "dev" structure and call it a day. If it succeeds, we proceed to create a device descriptor structure and add it to the list of open devices.
>>> This approach means that X11DeviceDriver() cannot do anything that potentially accesses the dd structure because it isn't there yet, and the things it cannot do apparently includes calling R_ProcessX11Events(). [To be completely sure that this is actually still true, I'd need to have a closer look at what handleEvent() does.]
>>> So to fix things, it would seem that you could (a) add the device before attempting to start the driver, preparing to back it out if the driver fails to start, or (b) add a call to R_ProcessX11Events() _after_ the GEaddDevice2(dd, devname). Option (b) is probably the easiest.
>>> Paul: Does this analysis look roughly right?
>>> -pd
>>>> On 26 Apr 2019, at 01:23 , [hidden email] wrote:
>>>>
>>>> Thanks Professor Dalgard.
>>>>
>>>> If you have a different way to fix the bug then I'd be happy to test
>>>> it.
>>>>
>>>> Or whatever. I understand that maybe some data was being referenced
>>>> before it had been initialized. I could also support moving the
>>>> R_ProcessEvents call in another place, but it seems one would also
>>>> like to generate some kind of warning message, at the location of the
>>>> bad reference, rather than segfaulting. Was it not possible to
>>>> identify this location? I'm guessing that Valgrind is a bit more
>>>> mature now than it was in 2001...?
>>>>
>>>> Frederick
>>>>
>>>> On Wed, Apr 24, 2019 at 03:12:55PM +0200, peter dalgaard wrote:
>>>>> OK, so I did the archaeology anyway....
>>>>>
>>>>>
>>>>> This was the story, R-core November 29, 2001. Part of thread "X11 still segfaults".
>>>>>
>>>>> ------------>>
>>>>> .....
>>>>> Gah. I've been too tired today. Why did that take me so long?
>>>>>
>>>>> The culprit seems to be
>>>>>
>>>>> R_ProcessEvents((void*) NULL)
>>>>>
>>>>> in newX11DeviceDriver
>>>>>
>>>>> This gets called *before* this stuff at the end of Rf_addX11Device
>>>>>
>>>>> dd = GEcreateDevDesc(dev);
>>>>> addDevice((DevDesc*) dd);
>>>>> initDisplayList((DevDesc*) dd);
>>>>>
>>>>> and it is that "dd" that gets called by Rf_playDisplayList. Removing
>>>>> the offending line stops the segfaulting, seemingly with no ill
>>>>> effects.
>>>>>
>>>>> I'm not really sure what the use of that line ever was; it might be
>>>>> necessary to make the call somewhere later, but it appears to have
>>>>> been possible to race past it before receiving any events all the
>>>>> time.
>>>>>
>>>>> I also changed a couple of spots missing dd->newDevStruct=1
>>>>>
>>>>> Will commit in a moment.
>>>>> <<------------
>>>>>
>>>>> And the following day, in "graphics saga part III", we had
>>>>>
>>>>> ------------->>
>>>>> ...
>>>>>
>>>>> I can't make it happen in 1.3.1 but...
>>>>>
>>>>> It is probably not unrelated to the R_ProcessEvents line that
>>>>> I took out, but that was definitely wrong. However, one might reenable
>>>>> it if one could change this bit of code
>>>>>
>>>>> if (!(ptr_X11DeviceDriver)((DevDesc*)(dev), display, width, height, ps, gamma,
>>>>>      colormodel, maxcubesize, canvascolor)) {
>>>>>    free(dev);
>>>>>    errorcall(gcall, "unable to start device %s", devname);
>>>>>       }
>>>>> gsetVar(install(".Device"), mkString(devname), R_NilValue);
>>>>> dd = GEcreateDevDesc(dev);
>>>>> addDevice((DevDesc*) dd);
>>>>> initDisplayList((DevDesc*) dd);
>>>>>
>>>>>
>>>>> and put the if-clause last. A cursory clance through the three
>>>>> functions that are being called didn't reveal anything that would rely
>>>>> on having opened the device driver first.
>>>>>
>>>>> Paul?
>>>>>
>>>>> (I might try it locally, but I'm not sure I should commit anything.)
>>>>>
>>>>> <<-----------
>>>>>
>>>>> It seems that the suggestion was never followed up on?
>>>>>
>>>>> -pd
>>>>>
>>>>>
>>>>>> On 24 Apr 2019, at 11:42 , peter dalgaard <[hidden email]> wrote:
>>>>>>
>>>>>> I don't recall exactly what I did 18 years ago eiher and I likely don't have the time to dig into the archives and reconstruct.
>>>>>>
>>>>>> I can imagine that the issue had to do with the protocol around creating and mapping windows. Presumably the segfault comes from looking for events on a window that hasn't been created yet, or has already been destroyed, leading to a NULL reference somewhere. I have a vague recollection that the issue was window manager dependent (in 2001 probably not twm, more likely xvwm on RedHat if it was affecting me).
>>>>>>
>>>>>> A proper fix should go via proper understanding of the X11 protocol - uncommenting a line is as bad as commenting it in the 1st place.... So more like "wait for window to exist THEN process events" -- but the 1st part may be WM specific, etc.
>>>>>>
>>>>>> I recall docs being quite obtuse, and the X11 "mechanism not policy" credo doesn't help as WMs are not obliged to (say) send notifications, so you can end up stalling, waiting for events that never happen.
>>>>>>
>>>>>> It is entirely possible that there is stuff in here that I didn't understand properly at the time, and still don't!
>>>>>>
>>>>>> - pd
>>>>>>
>>>>>>> On 24 Apr 2019, at 02:30 , Paul Murrell <[hidden email]> wrote:
>>>>>>>
>>>>>>> Hi
>>>>>>>
>>>>>>> Sorry, I can't offer an explanation for the commented-out line.
>>>>>>> However, regarding your final question of avoiding the R-core bottleneck, you do have the option of creating a third-party graphics device package.  See, for example, the 'tikzDevice' and 'svglite' packages on CRAN.  Does that provide you with a way forward ?
>>>>>>>
>>>>>>> Paul
>>>>>>>
>>>>>>> On 20/04/2019 5:27 p.m., [hidden email] wrote:
>>>>>>>> Dear R Devel,
>>>>>>>>
>>>>>>>> I know that someone put this line in src/modules/X11/devX11.c:2824 for
>>>>>>>> a reason, because commenting it out causes R to miss an important
>>>>>>>> ConfigureNotify event in my window manager. The result is that plots
>>>>>>>> are initially drawn off the window borders, unreadable.
>>>>>>>>
>>>>>>>>  R_ProcessX11Events((void*) NULL);
>>>>>>>>
>>>>>>>> Unfortunately for me, this line is commented in the standard release
>>>>>>>> of R, it has "#if BUG ... #endif" around it.
>>>>>>>>
>>>>>>>> I guess it is also unfortunate for anyone who uses the same window
>>>>>>>> manager as I do, namely i3, which I think is pretty popular among Unix
>>>>>>>> power users these days; not to mention other full-screen window
>>>>>>>> managers which probably exhibit the same bug in R.
>>>>>>>>
>>>>>>>> Maybe everyone on the Core team uses twm as their window manager? Or
>>>>>>>> RStudio on Windows? Which would be sad because then we're not
>>>>>>>> representing an important user demographic, namely those who prefer
>>>>>>>> software which is modern and powerful, yet simple to understand and
>>>>>>>> modify; fully configurable and interoperable and so on.
>>>>>>>>
>>>>>>>> I first reported this bug 3 years ago. In doing research for my bug
>>>>>>>> report, I found that the line was commented out by Peter Dalgaard in
>>>>>>>> 2001 with the explanation "X11 segfault fix - I hope".
>>>>>>>>
>>>>>>>> I don't know what the way forward is. Obviously the Core Team has
>>>>>>>> reason to say, "look, this isn't very important, it's been broken
>>>>>>>> since 2001, maybe fixing it will cause the undocumented segfault bug
>>>>>>>> to reappear, clearly no one here uses your window manager". Do I have
>>>>>>>> to submit a correctness proof for the proposed change? What do I do?
>>>>>>>>
>>>>>>>> https://bugs.r-project.org/bugzilla/show_bug.cgi?id=16702
>>>>>>>>
>>>>>>>> As mentioned in my bug report, I checked using gdb that
>>>>>>>> ConfigureNotify is indeed being received by the call to
>>>>>>>> R_ProcessX11Events() when it is uncommented. I haven't experienced any
>>>>>>>> segfaults.
>>>>>>>>
>>>>>>>> It's good that Peter left evidence that "R_ProcessX11Events" was being
>>>>>>>> called 18 years ago from X11DeviceDriver(). If he had deleted the
>>>>>>>> line, rather than commenting it, then discovering the reason for the
>>>>>>>> window rendering bug would have been much harder for me.
>>>>>>>>
>>>>>>>> However, the downside is that now it is not just a matter of inserting
>>>>>>>> the line where it belongs; I also feel a bit like I have to explain
>>>>>>>> why it was initially removed. But although I've given it some thought,
>>>>>>>> I still have no idea.
>>>>>>>>
>>>>>>>> Somewhat tangentially, I am wondering if there is some way that we
>>>>>>>> could make the development of R's graphics code proceed at a faster
>>>>>>>> rate, for example by pulling it out into a separate module, so that
>>>>>>>> people could offer alternative implementations via CRAN etc., rather
>>>>>>>> than having R Core be the bottleneck. Would this make sense? Has it
>>>>>>>> already been done?
>>>>>>>>
>>>>>>>> Thank you,
>>>>>>>>
>>>>>>>> Frederick
>>>>>>>>
>>>>>>>> ______________________________________________
>>>>>>>> [hidden email] mailing list
>>>>>>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>>>>
>>>>>>> --
>>>>>>> Dr Paul Murrell
>>>>>>> Department of Statistics
>>>>>>> The University of Auckland
>>>>>>> Private Bag 92019
>>>>>>> Auckland
>>>>>>> New Zealand
>>>>>>> 64 9 3737599 x85392
>>>>>>> [hidden email]
>>>>>>> http://www.stat.auckland.ac.nz/~paul/
>>>>>>>
>>>>>>> ______________________________________________
>>>>>>> [hidden email] mailing list
>>>>>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>>>
>>>>>> --
>>>>>> Peter Dalgaard, Professor,
>>>>>> Center for Statistics, Copenhagen Business School
>>>>>> Solbjerg Plads 3, 2000 Frederiksberg, Denmark
>>>>>> Phone: (+45)38153501
>>>>>> Office: A 4.23
>>>>>> Email: [hidden email]  Priv: [hidden email]
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> --
>>>>> Peter Dalgaard, Professor,
>>>>> Center for Statistics, Copenhagen Business School
>>>>> Solbjerg Plads 3, 2000 Frederiksberg, Denmark
>>>>> Phone: (+45)38153501
>>>>> Office: A 4.23
>>>>> Email: [hidden email]  Priv: [hidden email]
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>
>> --
>> Dr Paul Murrell
>> Department of Statistics
>> The University of Auckland
>> Private Bag 92019
>> Auckland
>> New Zealand
>> 64 9 3737599 x85392
>> [hidden email]
>> http://www.stat.auckland.ac.nz/~paul/
>
>--
>Peter Dalgaard, Professor,
>Center for Statistics, Copenhagen Business School
>Solbjerg Plads 3, 2000 Frederiksberg, Denmark
>Phone: (+45)38153501
>Office: A 4.23
>Email: [hidden email]  Priv: [hidden email]
>
>
>
>
>
>
>
>
>

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: [FORGED] src/modules/X11/devX11.c, can we remove "#if BUG" yet

frederik
Thanks again ... but by the way, I wonder if this would be a good time
to take another look at the use case that produced this error.

Consider that you have a complicated algorithm you are trying to
analyze, and you decide to display plots of certain data, as part of
the algorithm's main loop. Thanks to the recent fix, the plot window
will be correctly resized when it first opens and receives a
ConfigureNotify from the window manager. However, if the user decides
to resize it again while the algorithm is running, then the window
won't react until control comes back to the R read-eval-print loop,
since this is where R_ProcessX11Events gets called. Question: is it
possible to call R_ProcessX11Events manually from R code? That would
be another way to solve the problem. Or if R_ProcessX11Events is
called with every call to plot() or dev.flush() or something, might be
an even simpler solution.

Previously, R developers have been kind enough to add an "onIdle()"
handler to the getGraphicsEvent handlers, making it possible for me to
have interactive animated plots. I love this feature, but I think
there are some applications, like the above, where it would be more
convenient to keep a loop-based flow control.

I don't want to overwhelm you with requests, of course, so maybe this
goes on a queue to think about later.

Thanks,

Frederick

On Thu, May 02, 2019 at 02:07:19PM -0700, [hidden email] wrote:

>I tested it. It fixes the bug and didn't seem to produce any errors. Thank you Professor Dalgaard! I'm so glad this has finally been addressed. I will update the bug report.
>
>(https://bugs.r-project.org/bugzilla/show_bug.cgi?id=16702)
>
>On Thu, May 02, 2019 at 04:48:51PM +0200, peter dalgaard wrote:
>>OK, this is now in R-devel, but only superficially tested (b/c this is a Mac). Please check it out.
>>
>>-pd
>>
>>>On 30 Apr 2019, at 23:09 , Paul Murrell <[hidden email]> wrote:
>>>
>>>Hi Peter
>>>
>>>Yes, that looks roughly right to me.  I would be in favour of your option (b), partly because it is probably easiest and partly because that retains the basic graphics device startup logic pattern that is replicated across all(?) graphics devices.
>>>
>>>Paul
>>>
>>>On 28/04/19 11:39 AM, peter dalgaard wrote:
>>>>I had a look at the current code, and AFAICT it has essentially the same structure as it did back then. I think it may have finally dawned upon me what the issue really is:
>>>>The logic is that in Rf_addX11Device, we have
>>>>        if (!X11DeviceDriver(dev, display, width, height,
>>>>                             ps, gamma, colormodel, maxcubesize,
>>>>                             bgcolor, canvascolor, sfonts, res,
>>>>                             xpos, ypos, title, useCairo, antialias, family)) {
>>>>            free(dev);
>>>>            errorcall(call, _("unable to start device %s"), devname);
>>>>        }
>>>>        dd = GEcreateDevDesc(dev);
>>>>        GEaddDevice2(dd, devname);
>>>>i.e., we start the device driver, and if it fails, we throw away the "dev" structure and call it a day. If it succeeds, we proceed to create a device descriptor structure and add it to the list of open devices.
>>>>This approach means that X11DeviceDriver() cannot do anything that potentially accesses the dd structure because it isn't there yet, and the things it cannot do apparently includes calling R_ProcessX11Events(). [To be completely sure that this is actually still true, I'd need to have a closer look at what handleEvent() does.]
>>>>So to fix things, it would seem that you could (a) add the device before attempting to start the driver, preparing to back it out if the driver fails to start, or (b) add a call to R_ProcessX11Events() _after_ the GEaddDevice2(dd, devname). Option (b) is probably the easiest.
>>>>Paul: Does this analysis look roughly right?
>>>>-pd
>>>>>On 26 Apr 2019, at 01:23 , [hidden email] wrote:
>>>>>
>>>>>Thanks Professor Dalgard.
>>>>>
>>>>>If you have a different way to fix the bug then I'd be happy to test
>>>>>it.
>>>>>
>>>>>Or whatever. I understand that maybe some data was being referenced
>>>>>before it had been initialized. I could also support moving the
>>>>>R_ProcessEvents call in another place, but it seems one would also
>>>>>like to generate some kind of warning message, at the location of the
>>>>>bad reference, rather than segfaulting. Was it not possible to
>>>>>identify this location? I'm guessing that Valgrind is a bit more
>>>>>mature now than it was in 2001...?
>>>>>
>>>>>Frederick
>>>>>
>>>>>On Wed, Apr 24, 2019 at 03:12:55PM +0200, peter dalgaard wrote:
>>>>>>OK, so I did the archaeology anyway....
>>>>>>
>>>>>>
>>>>>>This was the story, R-core November 29, 2001. Part of thread "X11 still segfaults".
>>>>>>
>>>>>>------------>>
>>>>>>.....
>>>>>>Gah. I've been too tired today. Why did that take me so long?
>>>>>>
>>>>>>The culprit seems to be
>>>>>>
>>>>>>R_ProcessEvents((void*) NULL)
>>>>>>
>>>>>>in newX11DeviceDriver
>>>>>>
>>>>>>This gets called *before* this stuff at the end of Rf_addX11Device
>>>>>>
>>>>>> dd = GEcreateDevDesc(dev);
>>>>>> addDevice((DevDesc*) dd);
>>>>>> initDisplayList((DevDesc*) dd);
>>>>>>
>>>>>>and it is that "dd" that gets called by Rf_playDisplayList. Removing
>>>>>>the offending line stops the segfaulting, seemingly with no ill
>>>>>>effects.
>>>>>>
>>>>>>I'm not really sure what the use of that line ever was; it might be
>>>>>>necessary to make the call somewhere later, but it appears to have
>>>>>>been possible to race past it before receiving any events all the
>>>>>>time.
>>>>>>
>>>>>>I also changed a couple of spots missing dd->newDevStruct=1
>>>>>>
>>>>>>Will commit in a moment.
>>>>>><<------------
>>>>>>
>>>>>>And the following day, in "graphics saga part III", we had
>>>>>>
>>>>>>------------->>
>>>>>>...
>>>>>>
>>>>>>I can't make it happen in 1.3.1 but...
>>>>>>
>>>>>>It is probably not unrelated to the R_ProcessEvents line that
>>>>>>I took out, but that was definitely wrong. However, one might reenable
>>>>>>it if one could change this bit of code
>>>>>>
>>>>>> if (!(ptr_X11DeviceDriver)((DevDesc*)(dev), display, width, height, ps, gamma,
>>>>>>      colormodel, maxcubesize, canvascolor)) {
>>>>>>    free(dev);
>>>>>>    errorcall(gcall, "unable to start device %s", devname);
>>>>>>     }
>>>>>> gsetVar(install(".Device"), mkString(devname), R_NilValue);
>>>>>> dd = GEcreateDevDesc(dev);
>>>>>> addDevice((DevDesc*) dd);
>>>>>> initDisplayList((DevDesc*) dd);
>>>>>>
>>>>>>
>>>>>>and put the if-clause last. A cursory clance through the three
>>>>>>functions that are being called didn't reveal anything that would rely
>>>>>>on having opened the device driver first.
>>>>>>
>>>>>>Paul?
>>>>>>
>>>>>>(I might try it locally, but I'm not sure I should commit anything.)
>>>>>>
>>>>>><<-----------
>>>>>>
>>>>>>It seems that the suggestion was never followed up on?
>>>>>>
>>>>>>-pd
>>>>>>
>>>>>>
>>>>>>>On 24 Apr 2019, at 11:42 , peter dalgaard <[hidden email]> wrote:
>>>>>>>
>>>>>>>I don't recall exactly what I did 18 years ago eiher and I likely don't have the time to dig into the archives and reconstruct.
>>>>>>>
>>>>>>>I can imagine that the issue had to do with the protocol around creating and mapping windows. Presumably the segfault comes from looking for events on a window that hasn't been created yet, or has already been destroyed, leading to a NULL reference somewhere. I have a vague recollection that the issue was window manager dependent (in 2001 probably not twm, more likely xvwm on RedHat if it was affecting me).
>>>>>>>
>>>>>>>A proper fix should go via proper understanding of the X11 protocol - uncommenting a line is as bad as commenting it in the 1st place.... So more like "wait for window to exist THEN process events" -- but the 1st part may be WM specific, etc.
>>>>>>>
>>>>>>>I recall docs being quite obtuse, and the X11 "mechanism not policy" credo doesn't help as WMs are not obliged to (say) send notifications, so you can end up stalling, waiting for events that never happen.
>>>>>>>
>>>>>>>It is entirely possible that there is stuff in here that I didn't understand properly at the time, and still don't!
>>>>>>>
>>>>>>>- pd
>>>>>>>
>>>>>>>>On 24 Apr 2019, at 02:30 , Paul Murrell <[hidden email]> wrote:
>>>>>>>>
>>>>>>>>Hi
>>>>>>>>
>>>>>>>>Sorry, I can't offer an explanation for the commented-out line.
>>>>>>>>However, regarding your final question of avoiding the R-core bottleneck, you do have the option of creating a third-party graphics device package.  See, for example, the 'tikzDevice' and 'svglite' packages on CRAN.  Does that provide you with a way forward ?
>>>>>>>>
>>>>>>>>Paul
>>>>>>>>
>>>>>>>>On 20/04/2019 5:27 p.m., [hidden email] wrote:
>>>>>>>>>Dear R Devel,
>>>>>>>>>
>>>>>>>>>I know that someone put this line in src/modules/X11/devX11.c:2824 for
>>>>>>>>>a reason, because commenting it out causes R to miss an important
>>>>>>>>>ConfigureNotify event in my window manager. The result is that plots
>>>>>>>>>are initially drawn off the window borders, unreadable.
>>>>>>>>>
>>>>>>>>> R_ProcessX11Events((void*) NULL);
>>>>>>>>>
>>>>>>>>>Unfortunately for me, this line is commented in the standard release
>>>>>>>>>of R, it has "#if BUG ... #endif" around it.
>>>>>>>>>
>>>>>>>>>I guess it is also unfortunate for anyone who uses the same window
>>>>>>>>>manager as I do, namely i3, which I think is pretty popular among Unix
>>>>>>>>>power users these days; not to mention other full-screen window
>>>>>>>>>managers which probably exhibit the same bug in R.
>>>>>>>>>
>>>>>>>>>Maybe everyone on the Core team uses twm as their window manager? Or
>>>>>>>>>RStudio on Windows? Which would be sad because then we're not
>>>>>>>>>representing an important user demographic, namely those who prefer
>>>>>>>>>software which is modern and powerful, yet simple to understand and
>>>>>>>>>modify; fully configurable and interoperable and so on.
>>>>>>>>>
>>>>>>>>>I first reported this bug 3 years ago. In doing research for my bug
>>>>>>>>>report, I found that the line was commented out by Peter Dalgaard in
>>>>>>>>>2001 with the explanation "X11 segfault fix - I hope".
>>>>>>>>>
>>>>>>>>>I don't know what the way forward is. Obviously the Core Team has
>>>>>>>>>reason to say, "look, this isn't very important, it's been broken
>>>>>>>>>since 2001, maybe fixing it will cause the undocumented segfault bug
>>>>>>>>>to reappear, clearly no one here uses your window manager". Do I have
>>>>>>>>>to submit a correctness proof for the proposed change? What do I do?
>>>>>>>>>
>>>>>>>>>https://bugs.r-project.org/bugzilla/show_bug.cgi?id=16702
>>>>>>>>>
>>>>>>>>>As mentioned in my bug report, I checked using gdb that
>>>>>>>>>ConfigureNotify is indeed being received by the call to
>>>>>>>>>R_ProcessX11Events() when it is uncommented. I haven't experienced any
>>>>>>>>>segfaults.
>>>>>>>>>
>>>>>>>>>It's good that Peter left evidence that "R_ProcessX11Events" was being
>>>>>>>>>called 18 years ago from X11DeviceDriver(). If he had deleted the
>>>>>>>>>line, rather than commenting it, then discovering the reason for the
>>>>>>>>>window rendering bug would have been much harder for me.
>>>>>>>>>
>>>>>>>>>However, the downside is that now it is not just a matter of inserting
>>>>>>>>>the line where it belongs; I also feel a bit like I have to explain
>>>>>>>>>why it was initially removed. But although I've given it some thought,
>>>>>>>>>I still have no idea.
>>>>>>>>>
>>>>>>>>>Somewhat tangentially, I am wondering if there is some way that we
>>>>>>>>>could make the development of R's graphics code proceed at a faster
>>>>>>>>>rate, for example by pulling it out into a separate module, so that
>>>>>>>>>people could offer alternative implementations via CRAN etc., rather
>>>>>>>>>than having R Core be the bottleneck. Would this make sense? Has it
>>>>>>>>>already been done?
>>>>>>>>>
>>>>>>>>>Thank you,
>>>>>>>>>
>>>>>>>>>Frederick
>>>>>>>>>
>>>>>>>>>______________________________________________
>>>>>>>>>[hidden email] mailing list
>>>>>>>>>https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>>>>>
>>>>>>>>--
>>>>>>>>Dr Paul Murrell
>>>>>>>>Department of Statistics
>>>>>>>>The University of Auckland
>>>>>>>>Private Bag 92019
>>>>>>>>Auckland
>>>>>>>>New Zealand
>>>>>>>>64 9 3737599 x85392
>>>>>>>>[hidden email]
>>>>>>>>http://www.stat.auckland.ac.nz/~paul/
>>>>>>>>
>>>>>>>>______________________________________________
>>>>>>>>[hidden email] mailing list
>>>>>>>>https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>>>>
>>>>>>>--
>>>>>>>Peter Dalgaard, Professor,
>>>>>>>Center for Statistics, Copenhagen Business School
>>>>>>>Solbjerg Plads 3, 2000 Frederiksberg, Denmark
>>>>>>>Phone: (+45)38153501
>>>>>>>Office: A 4.23
>>>>>>>Email: [hidden email]  Priv: [hidden email]
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>--
>>>>>>Peter Dalgaard, Professor,
>>>>>>Center for Statistics, Copenhagen Business School
>>>>>>Solbjerg Plads 3, 2000 Frederiksberg, Denmark
>>>>>>Phone: (+45)38153501
>>>>>>Office: A 4.23
>>>>>>Email: [hidden email]  Priv: [hidden email]
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>
>>>--
>>>Dr Paul Murrell
>>>Department of Statistics
>>>The University of Auckland
>>>Private Bag 92019
>>>Auckland
>>>New Zealand
>>>64 9 3737599 x85392
>>>[hidden email]
>>>http://www.stat.auckland.ac.nz/~paul/
>>
>>--
>>Peter Dalgaard, Professor,
>>Center for Statistics, Copenhagen Business School
>>Solbjerg Plads 3, 2000 Frederiksberg, Denmark
>>Phone: (+45)38153501
>>Office: A 4.23
>>Email: [hidden email]  Priv: [hidden email]
>>
>>
>>
>>
>>
>>
>>
>>
>>
>

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel