Handle conflicts between epoxy and manually resolved function pointers

Reviews
Diff

- Download Diff

Information
Submitter:	Martin Flöser
Repository:	kwin
Branch:	master
Bugs:
Depends On:
Commit:	102d325...
Reviewers
Groups:	kwin
People:	garg

Description

For the functions from GL_FOO_robustness we want to resolve it by
ourselves in order to add a custom implementation if it's not available.
Unfortunately once epoxy.h is included this breaks as epoxy defines the
names and so through the preprocessor epoxy always wins.

So we need different names: all functions from robustness get a "kwin"
prefix and the usage is changed everywhere in kwin source code.

This is on top of http://commits.kde.org/clones/kwin/graesslin/kwin/7eaad2e6544e2437a69a27d186af6f6653f4e391

Testing Done

Looks good to me from my limited knowledge about KWin.

convincing "using KWin::glReadnPixels;" etc. in some relevant header?

epoxy will "win" because the functions are in "::", though that's really weird, since at lease eg. the shader test is in the KWin namespace - so there should be an ambiguity error on compilation?!
Otherwise rather resort to explicit namespace resolution (should also be ABI stable)?

kwinGlReadnPixels -> KWin::glReadnPixels

Martin Flöser Nov. 2, 2015, 7:18 a.m.

I tried some of that. epoxy provides it as a define:
#define glReadnPixels epoxy_glReadnPixels

so a direct KWin::glReadnPixels get turned into KWin::epoxy_glReadnPixels by the precompiler -> doesn't compile :-(

I also gave a patch with
#undef glReadnPixels
in kwinglutils_funcs.h after including epoxy

to Rohan and that didn't work. I guess it might end up in more places. And that approach just askes for unexpected failure.

convincing "using KWin::glReadnPixels;" etc. in some relevant header?

That's something I didn't try, but given the experience with the define I'm sure it won't work.

If you have other ideas for fooling the preproc I'm all ears :-)

Thomas Lübking Nov. 2, 2015, 3:32 p.m.

Yikes. It's close to impossible to "steal" the definition, as epoxy is "#pragma once", but gl_generated.h "re"defines glReadnPixels to epoxy_glReadnPixels, so that already /is/ KWin::epoxy_glReadnPixels (try by turning

glReadnPixels_func            glReadnPixels;

into

glReadnPixels_func            epoxy_glReadnPixels;

in the source, should still compile fine) so "using KWin::glReadnPixels;" is indeed "using KWin::epoxy_glReadnPixels;" (the function exists twice in different namespaces) and that should work

Just tried (forcing down robustness) and KWin::ReadnPixels is resolved from the shader selftest, even w/o special using instruction:

diff --git a/libkwineffects/kwinglutils.cpp b/libkwineffects/kwinglutils.cpp
index 7604554..741f4fc 100644
--- a/libkwineffects/kwinglutils.cpp
+++ b/libkwineffects/kwinglutils.cpp
@@ -679,7 +679,9 @@ static bool fuzzyCompare(const QVector4D &lhs, const QVector4D &rhs)
 static bool checkPixel(int x, int y, const QVector4D &expected, const char *file, int line)
 {
     uint8_t data[4];
+    qDebug() << "check pixel";
     glReadnPixels(x, y, 1, 1, GL_RGBA, GL_UNSIGNED_BYTE, 4, data);
+    qDebug() << "correct?";

     const QVector4D pixel{data[0] / 255.f, data[1] / 255.f, data[2] / 255.f, data[3] / 255.f};

diff --git a/libkwineffects/kwinglutils_funcs.cpp b/libkwineffects/kwinglutils_funcs.cpp
index dfb673b..e2bb460 100644
--- a/libkwineffects/kwinglutils_funcs.cpp
+++ b/libkwineffects/kwinglutils_funcs.cpp
@@ -81,7 +81,7 @@ void eglResolveFunctions()
 void glResolveFunctions(OpenGLPlatformInterface platformInterface)
 {
 #ifndef KWIN_HAVE_OPENGLES
-    if (hasGLExtension(QByteArrayLiteral("GL_ARB_robustness"))) {
+    if (false && hasGLExtension(QByteArrayLiteral("GL_ARB_robustness"))) {
         // See http://www.opengl.org/registry/specs/ARB/robustness.txt
         GL_RESOLVE_WITH_EXT(glGetGraphicsResetStatus, glGetGraphicsResetStatusARB);
         GL_RESOLVE_WITH_EXT(glReadnPixels,            glReadnPixelsARB);
@@ -113,6 +113,7 @@ static GLenum GetGraphicsResetStatus()
 static void ReadnPixels(GLint x, GLint y, GLsizei width, GLsizei height, GLenum format,
                         GLenum type, GLsizei bufSize, GLvoid *data)
 {
+    qDebug() << "boing";
     Q_UNUSED(bufSize)
     glReadPixels(x, y, width, height, format, type, data);
 }


So this will be something special about the namespace resolution in the specific compilation?

=> Rohan, please try to convince for "using KWin::epoxy_glReadnPixels;" [sic!, should work fine] in kwinglutils.cpp (I don't even understand why it's not resolved itfp, we're in KWin namespace there...)

Rohan Garg Nov. 3, 2015, 2:32 p.m.

I just tested your patch and I do not see a "boing" on my terminal.

Here's the full output : https://paste.kde.org/pjg61rapt

Thomas Lübking Nov. 3, 2015, 3:18 p.m.

That's the problem, yes (though weird)
try to replace

- glReadnPixels(x, y, 1, 1, GL_RGBA, GL_UNSIGNED_BYTE, 4, data);
+ KWin::glReadnPixels(x, y, 1, 1, GL_RGBA, GL_UNSIGNED_BYTE, 4, data);

and whether that get's you a "boing" between "check pixel" and "correct"

Rohan Garg Nov. 3, 2015, 3:26 p.m.
```
I do not.
```

Thomas Lübking Nov. 3, 2015, 3:39 p.m.

Not try or not get a boing?
If it compiles and you don't get a boing, that smells like a bug in the linker, since KWin::foo() should certainly not resolve ::foo() ...

I could assume the build chanin cannot handle the (unofficial) "#pragma once" correctly (would you require a patch for epoxy to replace the pragma by the classical "#ifndef #define #endif" stance?)

Rohan Garg Nov. 3, 2015, 3:43 p.m.

I did not get a boing :)

It compiles just fine btw. I would not be surprised to see if this was a linker bug, I've experienced linker bugs with Qt/armhf before, so it's entirely within the realm of possibilities.

Thomas Lübking Nov. 3, 2015, 10:22 p.m.

This replaces the #pragma once in /usr/include/epoxy (in case you want to try whether that's the cause of the wrong resolution)


diff -pru /usr/include/epoxy/egl_generated.h /tmp/epoxy/egl_generated.h
--- /usr/include/epoxy/egl_generated.h  2015-07-20 12:26:02.000000000 +0200
+++ /tmp/epoxy/egl_generated.h  2015-11-03 23:18:20.406599483 +0100
@@ -2,7 +2,9 @@
  * This is code-generated from the GL API XML files from Khronos.
  */

-#pragma once
+#ifndef EPOXY_EGL_GENERATED_H
+#define EPOXY_EGL_GENERATED_H
+
 #include <inttypes.h>
 #include <stddef.h>

@@ -985,3 +987,5 @@ extern EPOXY_IMPORTEXPORT EGLint (EPOXY_
 #define eglWaitNative epoxy_eglWaitNative
 #define eglWaitSync epoxy_eglWaitSync
 #define eglWaitSyncKHR epoxy_eglWaitSyncKHR
+
+#endif // EPOXY_EGL_GENERATED_H
\ No newline at end of file
diff -pru /usr/include/epoxy/gl_generated.h /tmp/epoxy/gl_generated.h
--- /usr/include/epoxy/gl_generated.h   2015-07-20 12:26:02.000000000 +0200
+++ /tmp/epoxy/gl_generated.h   2015-11-03 23:18:12.199890528 +0100
@@ -24,7 +24,9 @@
  * 
  */

-#pragma once
+#ifndef EPOXY_GL_GENERATED_H
+#define EPOXY_GL_GENERATED_H
+
 #include <inttypes.h>
 #include <stddef.h>

@@ -18688,3 +18690,5 @@ extern EPOXY_IMPORTEXPORT void (EPOXY_CA
 #define glWindowPos4sMESA epoxy_glWindowPos4sMESA
 #define glWindowPos4svMESA epoxy_glWindowPos4svMESA
 #define glWriteMaskEXT epoxy_glWriteMaskEXT
+
+#endif // EPOXY_GL_GENERATED_H
\ No newline at end of file
diff -pru /usr/include/epoxy/glx_generated.h /tmp/epoxy/glx_generated.h
--- /usr/include/epoxy/glx_generated.h  2015-07-20 12:26:02.000000000 +0200
+++ /tmp/epoxy/glx_generated.h  2015-11-03 23:18:26.836632699 +0100
@@ -2,7 +2,9 @@
  * This is code-generated from the GL API XML files from Khronos.
  */

-#pragma once
+#ifndef EPOXY_GLX_GENERATED_H
+#define EPOXY_GLX_GENERATED_H
+
 #include <inttypes.h>
 #include <stddef.h>

@@ -967,3 +969,5 @@ extern EPOXY_IMPORTEXPORT void (EPOXY_CA
 #define glXWaitGL epoxy_glXWaitGL
 #define glXWaitVideoSyncSGI epoxy_glXWaitVideoSyncSGI
 #define glXWaitX epoxy_glXWaitX
+
+#endif // EPOXY_GLX_GENERATED_H
\ No newline at end of file

Rohan Garg Nov. 3, 2015, 10:47 p.m.

Nope, still no boing, ( this is using KWin:: namespace patch btw )

Martin Flöser Nov. 4, 2015, 6:58 a.m.

so that sounds like my original patch is the way to go: @Rohan: did you test that one?

Thomas Lübking Nov. 4, 2015, 8:59 a.m.

Wow.
https://bugreports.qt.io/browse/QTBUG-47350

I'm pretty sure that the patch helps to bypass this issue, but if the linker resolves functions from namespaces as it pleases it, expect for more issues.

@Rohan, you're not trying to compile -Bsymbolic, are you?

Rohan Garg Nov. 4, 2015, 9:04 a.m.

Right, that Bsymbolic bug was what i was taking about earlier.

im not compiling with Bsymbolic explicitly, so I'll try and figure out if it's implicit in the build

Rohan Garg Nov. 4, 2015, 10:04 a.m.

@Thomas I didn't see -Bsymbolic anywhere in the build output ( I built it with make VERBOSE=1 ), so it's probably not the same issue as Qt.

Rohan Garg Nov. 5, 2015, 9:34 p.m.
```
So, where do we go from here?
```

Thomas Lübking Nov. 5, 2015, 9:38 p.m.

check this patch (the linker will hopefully not mess up different names, so it should work), then we can use it as workaround for this issue.

But if the linker stumbles on namespaces, expect random problems everywhere; ultimately you want the linkers symbol resolution fixed. (beyond -Bsymbolic, which is troublesome by nature anyway)

Rohan Garg Nov. 6, 2015, 12:33 p.m.

I'm actually going to try if your patch works with the gold linker first.

Rohan Garg Nov. 6, 2015, 3:29 p.m.
```
Didn't seem to make a difference :(
```

Thomas Lübking Nov. 7, 2015, 8:28 a.m.

This looks relevant, but it's very odd to have the same bug in GNU ld and gold...
https://answers.launchpad.net/gcc-arm-embedded/+question/224550

Passing "-mlong-calls" is claimed to work around it (on GNU ld)

Thomas Lübking Nov. 7, 2015, 8:29 a.m.

(ps: yes, it's claimed fixed - nevertheless ;-)

so what do we do? Should we ship this one?

Thomas Lübking Nov. 6, 2015, 8:10 a.m.

if it works for rohan? sure, one problem less - but we cannot scratch all pot. namespace conflicts in kde, let alone the rest of the system; so rohan really wants the linker fixed itr and the long run.

Rohan Garg Nov. 10, 2015, 7:33 p.m.

This patch seems to work, I still get :

kwin_core: Failed to initialize compositing, compositing disabled

But atleast the shader manager test seems to pass. ( I don't get a error message about the shader manager test ).

Status: Closed (submitted)

Change Summary:

Submitted with commit ac1dbac8b11f66a10e7871ae878e30a3e5d42100 by Martin Gräßlin to branch master.

Today I investigated another issue together with Rohan and we figured out that all is stupid. We were looking at a completely wrong problem: the driver supports robustness, the problem is that we don't create a robust context.

You have a pending review.

Review Board 2.5.15

Screenshots

Files

Status: Closed (submitted)

Change Summary: