Bug#712880: [Beignet] *** SPAM LEVEL 5.929 *** beignet: libEGL ABI abuse (getting at symbols not intended to be public)
Thanks for the test result. The attached is v2 patch to fix
the crash when run it without X server. Could you help to
test it again?
And Simon, could you also give this patch a try? Thanks.
On Fri, Jul 05, 2013 at 05:15:34PM +0800, He Junyan wrote:
> Hi Zhigang:
>
> I think this problem casued by the X server process.
> On my platform, I prefer not to run X by default, and there is no
> Xorg process. This may cause the EGL crash problem.
> When Xorg is running, all the cases are successful including
> compiler_fill_gl_image.
>
>
> On 07/05/2013 04:37 PM, He Junyan wrote:
> >Hi zhigang:
> >
> >This patch seems have some problem to me.
> >The unit test will crash every time,
> >
> >Program received signal SIGSEGV, Segmentation fault.
> >cl_gbm_set_image_extension (gbm=0x618640, display=0x0) at
> >/home/robinhe/CL/beignet/src/x11/gbm_dri2_x11_platform.c:99
> >99 struct _hack_dri2_egl_display *dri2_dpy = (struct
> >_hack_dri2_egl_display*)egl_dpy->DriverData;
> >(gdb) bt
> >#0 cl_gbm_set_image_extension (gbm=0x618640, display=0x0) at
> >/home/robinhe/CL/beignet/src/x11/gbm_dri2_x11_platform.c:99
> >#1 0x00007ffff6e34689 in intel_driver_open (intel=0x615e50,
> >props=0x7fffffffd0d0) at
> >/home/robinhe/CL/beignet/src/intel/intel_driver.c:216
> >#2 0x00007ffff6e34879 in cl_intel_driver_new
> >(props=0x7fffffffd0d0) at
> >/home/robinhe/CL/beignet/src/intel/intel_driver.c:378
> >#3 0x00007ffff6e30cb0 in cl_context_new (props=0x7fffffffd0d0) at
> >/home/robinhe/CL/beignet/src/cl_context.c:145
> >#4 0x00007ffff6e30e92 in cl_create_context (properties=0x618600,
> >num_devices=<optimized out>, devices=0x7ffff7dd9d80,
> >pfn_notify=<optimized out>, user_data=<optimized out>,
> > errcode_ret=0x7fffffffd14c) at
> >/home/robinhe/CL/beignet/src/cl_context.c:116
> >#5 0x00007ffff6e299f1 in clCreateContext (properties=0x618600,
> >num_devices=1, devices=0x7ffff7dd9d80, pfn_notify=0,
> >user_data=<optimized out>, errcode_ret=0x7fffffffdab4)
> > at /home/robinhe/CL/beignet/src/cl_api.c:203
> >#6 0x00007ffff7bbd811 in cl_ocl_init () at
> >/home/robinhe/CL/beignet/utests/utest_helper.cpp:358
> >#7 0x0000000000401172 in main (argc=1, argv=0x7fffffffe438) at
> >/home/robinhe/CL/beignet/utests/utest_run.cpp:33
> >(gdb)
> >
> >It seems the display struct may be changed between our mesa version.
> >
> >
> >On 07/05/2013 04:16 PM, Zhigang Gong wrote:
> >>Sorry, I just found I sent the wrong patch in my last email. Please
> >>ignore that patch and use this one:
> >>0001-CLGL-Refine-the-hack-of-gbm-extension-initialization.patch.
> >>
> >>On Thu, Jul 04, 2013 at 08:26:09PM +0800, Zhigang Gong wrote:
> >>>On Wed, Jul 03, 2013 at 01:35:08PM +0200, Simon Richter wrote:
> >>>>Hi,
> >>>>
> >>>>On 03.07.2013 12:01, Zhigang Gong wrote:
> >>>>
> >>>>>The gbm device doesn't init lookup_image method and the user data. The lookup_image is only
> >>>>>initialized when use the gbm device to create an egl display and then initialize the egl drm
> >>>>>platform which is not our use model.
> >>>>My plan for Beignet in Debian is to upload the releases to the regular
> >>>>track where they can move into the next release, and direct git
> >>>>snapshots at the experimental track (I've made an exception with 0.1,
> >>>>because that version breaks installed OpenCL software with the error
> >>>>returns from the query APIs), so ideally I'd like to have a solution
> >>>>before that.
> >>>>
> >>>>Can this be delegated to the Mesa project by means of a change request
> >>>>in their BTS?
> >>>I will do that when I have time, may be within this month. Before that,
> >>>I have a workaround patch to get the gl texture sharing work. Could you
> >>>help to test it? It's still very hacky but avoid reference to the mesa's
> >>>internal symbol directly.
> >>>
> >>>>Current state:
> >>>>
> >>>> - Debian#712880 (autogenerated dependencies too weak) blocks Mesa 9
> >>>>from propagating along the regular package track (unstable -> testing ->
> >>>>stable). I expect that to be fixed soon.
> >>>>
> >>>> - Debian#712903 (dependency on a non-public symbol) blocks Beignet from
> >>>>propagating within Debian
> >>>>
> >>>> - Debian#630344 (support for private symbols in package dependency
> >>>>calculations) is looming above our heads. When implemented, this feature
> >>>>will make builds of packages using private symbols from other packages fail.
> >>>>
> >>>>What I can do is drop EGL support in the Debian packages, at least for
> >>>>the regular track, which would allow us to be part of the release, but
> >>>>at reduced functionality.
> >>>>
> >>>> Simon
> >>>>
> >>>>Bug references:
> >>>>
> >>>>http://bugs.debian.org/712880
> >>>>http://bugs.debian.org/712903
> >>>>http://buge.debian.org/630344
> >>>>
> >>>
> >>>>_______________________________________________
> >>>>Beignet mailing list
> >>>>Beignet@lists.freedesktop.org
> >>>>http://lists.freedesktop.org/mailman/listinfo/beignet
> >>> From cacf883a0fed9e56beb943edb1f9e43ed87c928b Mon Sep 17 00:00:00 2001
> >>>From: Zhigang Gong<zhigang.gong@linux.intel.com>
> >>>Date: Thu, 4 Jul 2013 19:12:52 +0800
> >>>Subject: [PATCH] GBE: Clear the value map when start a new scalarize pass.
> >>>
> >>>The scalarize pass is a function pass, and the valueMap should
> >>>be a per-function data rather than a per-unit data. The reason
> >>>we put it in the unit data structure is that the scalarize pass
> >>>is before the GenWriter pass thus there is no ir::Function exists.
> >>>
> >>>As there may be multiple kernel functions in one unit, if we don't
> >>>clear the valueMap each time running a new scalarize pass, the previous
> >>>data may cause some unexpected behaviour. For example, the previous
> >>>instructions have been already erased, then latter a new instruction
> >>>in this function may be created in the same position of the erased
> >>>instruction, then it breaks this valueMap. That's the root cause why
> >>>we run the unit test several times and may encounter an assertion
> >>>sometime.
> >>>
> >>>This commit also modify the ir::unit layer implementation to remove
> >>>the dependency of llvm from that layer. In general, we should not add
> >>>llvm related code to the ir layer.
> >>>
> >>>Signed-off-by: Zhigang Gong<zhigang.gong@linux.intel.com>
> >>>---
> >>> backend/src/ir/unit.cpp | 14 --------------
> >>> backend/src/ir/unit.hpp | 20 +++++++-------------
> >>> backend/src/llvm/llvm_gen_backend.cpp | 7 ++++---
> >>> backend/src/llvm/llvm_scalarize.cpp | 2 +-
> >>> 4 files changed, 12 insertions(+), 31 deletions(-)
> >>>
> >>>diff --git a/backend/src/ir/unit.cpp b/backend/src/ir/unit.cpp
> >>>index 01e1eb1..4aeffe9 100644
> >>>--- a/backend/src/ir/unit.cpp
> >>>+++ b/backend/src/ir/unit.cpp
> >>>@@ -21,12 +21,6 @@
> >>> * \file unit.cpp
> >>> * \author Benjamin Segovia<benjamin.segovia@intel.com>
> >>> */
> >>>-#include "llvm/Config/config.h"
> >>>-#if LLVM_VERSION_MINOR <= 2
> >>>-#include "llvm/Instructions.h"
> >>>-#else
> >>>-#include "llvm/IR/Instructions.h"
> >>>-#endif /* LLVM_VERSION_MINOR <= 2 */
> >>> #include "ir/unit.hpp"
> >>> #include "ir/function.hpp"
> >>>@@ -59,14 +53,6 @@ namespace ir {
> >>> constantSet.append(data, name, size, alignment);
> >>> }
> >>>- void Unit::removeDeadValues()
> >>>- {
> >>>- for(auto &it : valueMap) {
> >>>- llvm::Instruction* I = llvm::dyn_cast<llvm::Instruction>(it.first.first); //fake value
> >>>- if((I == NULL) || (I->getParent() == NULL))
> >>>- valueMap.erase(it.first);
> >>>- }
> >>>- }
> >>> std::ostream &operator<< (std::ostream &out, const Unit &unit) {
> >>> unit.apply([&out] (const Function &fn) { out << fn << std::endl; });
> >>> return out;
> >>>diff --git a/backend/src/ir/unit.hpp b/backend/src/ir/unit.hpp
> >>>index 1017f5f..9e3d66a 100644
> >>>--- a/backend/src/ir/unit.hpp
> >>>+++ b/backend/src/ir/unit.hpp
> >>>@@ -24,13 +24,6 @@
> >>> #ifndef __GBE_IR_UNIT_HPP__
> >>> #define __GBE_IR_UNIT_HPP__
> >>>-#include "llvm/Config/config.h"
> >>>-#if LLVM_VERSION_MINOR <= 2
> >>>-#include "llvm/Value.h"
> >>>-#else
> >>>-#include "llvm/IR/Value.h"
> >>>-#endif /* LLVM_VERSION_MINOR <= 2 */
> >>>-
> >>> #include "ir/constant.hpp"
> >>> #include "ir/register.hpp"
> >>> #include "sys/hash_map.hpp"
> >>>@@ -49,7 +42,7 @@ namespace ir {
> >>> {
> >>> public:
> >>> typedef hash_map<std::string, Function*> FunctionSet;
> >>>- typedef std::pair<llvm::Value*, uint32_t> ValueIndex;
> >>>+ typedef std::pair<void*, uint32_t> ValueIndex;
> >>> /*! Create an empty unit */
> >>> Unit(PointerSize pointerSize = POINTER_32_BITS);
> >>> /*! Release everything (*including* the function pointers) */
> >>>@@ -84,8 +77,8 @@ namespace ir {
> >>> /*! Some values will not be allocated. For example a vector extract and
> >>> * a vector insertion when scalarize the vector load/store
> >>> */
> >>>- void newValueProxy(llvm::Value *real,
> >>>- llvm::Value *fake,
> >>>+ void newValueProxy(void *real,
> >>>+ void *fake,
> >>> uint32_t realIndex = 0u,
> >>> uint32_t fakeIndex = 0u) {
> >>> const ValueIndex key(fake, fakeIndex);
> >>>@@ -93,10 +86,11 @@ namespace ir {
> >>> GBE_ASSERT(valueMap.find(key) == valueMap.end()); // Do not insert twice
> >>> valueMap[key] = value;
> >>> }
> >>>- /* remove fake values that removed by other pass */
> >>>- void removeDeadValues(void);
> >>>+
> >>>+ void clearValueMap() { valueMap.clear(); }
> >>>+
> >>> /*! Return the value map */
> >>>- const map<ValueIndex, ValueIndex>& getValueMap(void) const { return valueMap; }
> >>>+ const map<ValueIndex, ValueIndex> &getValueMap(void) const { return valueMap; }
> >>> private:
> >>> friend class ContextInterface; //!< Can free modify the unit
> >>> hash_map<std::string, Function*> functions; //!< All the defined functions
> >>>diff --git a/backend/src/llvm/llvm_gen_backend.cpp b/backend/src/llvm/llvm_gen_backend.cpp
> >>>index 40f2667..36d8129 100644
> >>>--- a/backend/src/llvm/llvm_gen_backend.cpp
> >>>+++ b/backend/src/llvm/llvm_gen_backend.cpp
> >>>@@ -307,8 +307,10 @@ namespace gbe
> >>> }
> >>> /*! After scalarize pass, there are some valueMap in unit,
> >>> * use this function to copy from unit valueMap */
> >>>- void initValueMap(const map<ValueIndex, ValueIndex>& vMap) {
> >>>- valueMap.insert(vMap.begin(), vMap.end());
> >>>+ void initValueMap(const map<ir::Unit::ValueIndex, ir::Unit::ValueIndex> &vMap) {
> >>>+ for(auto &it : vMap)
> >>>+ newValueProxy((Value*)it.second.first, (Value*)it.first.first,
> >>>+ it.second.second, it.first.second);
> >>> }
> >>> /*! Mostly used for the preallocated registers (lids, gids) */
> >>> void newScalarProxy(ir::Register reg, Value *value, uint32_t index = 0u) {
> >>>@@ -1207,7 +1209,6 @@ namespace gbe
> >>> }
> >>> ctx.startFunction(F.getName());
> >>>- unit.removeDeadValues();
> >>> this->regTranslator.clear();
> >>> this->regTranslator.initValueMap(unit.getValueMap());
> >>> this->labelMap.clear();
> >>>diff --git a/backend/src/llvm/llvm_scalarize.cpp b/backend/src/llvm/llvm_scalarize.cpp
> >>>index bab2236..41674b6 100644
> >>>--- a/backend/src/llvm/llvm_scalarize.cpp
> >>>+++ b/backend/src/llvm/llvm_scalarize.cpp
> >>>@@ -773,7 +773,7 @@ namespace gbe {
> >>> intTy = IntegerType::get(module->getContext(), 32);
> >>> floatTy = Type::getFloatTy(module->getContext());
> >>> builder = new IRBuilder<>(module->getContext());
> >>>- unit.removeDeadValues();
> >>>+ unit.clearValueMap();
> >>> scalarizeArgs(F);
> >>> typedef ReversePostOrderTraversal<Function*> RPOTType;
> >>>--
> >>>1.7.9.5
> >>>
> >>>_______________________________________________
> >>>Beignet mailing list
> >>>Beignet@lists.freedesktop.org
> >>>http://lists.freedesktop.org/mailman/listinfo/beignet
> >>
> >>
> >>_______________________________________________
> >>Beignet mailing list
> >>Beignet@lists.freedesktop.org
> >>http://lists.freedesktop.org/mailman/listinfo/beignet
> >
> >
> >
> >_______________________________________________
> >Beignet mailing list
> >Beignet@lists.freedesktop.org
> >http://lists.freedesktop.org/mailman/listinfo/beignet
>
> _______________________________________________
> Beignet mailing list
> Beignet@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet
>From 541d46e46b0bcefa1e3a6952885f3fed3cc84345 Mon Sep 17 00:00:00 2001
From: Zhigang Gong <zhigang.gong@linux.intel.com>
Date: Thu, 4 Jul 2013 20:15:08 +0800
Subject: [PATCH v2] CLGL: Refine the hack of gbm extension initialization.
Previous implementation need to refer a EGL internal symbol.
This refinement is also a hack. It just avoid use the EGL internal
symbol, but it does use the internal EGL data structure.
Anyway, before we made some changes to gbm to support our use
model, this ugly method seems the only way to achive sharing
gl 2d/3d textures.
v2: This patch also fix the bug when it failed to get a valid egl
context it may crash.
Signed-off-by: Zhigang Gong <zhigang.gong@linux.intel.com>
---
src/intel/intel_driver.c | 1 +
src/x11/gbm_dri2_x11_platform.c | 103 +++++++++++++++++++++++++++++++++++--
utests/compiler_fill_gl_image.cpp | 4 ++
utests/utest_helper.cpp | 21 ++++----
4 files changed, 116 insertions(+), 13 deletions(-)
diff --git a/src/intel/intel_driver.c b/src/intel/intel_driver.c
index ebc4961..6c6b9fb 100644
--- a/src/intel/intel_driver.c
+++ b/src/intel/intel_driver.c
@@ -208,6 +208,7 @@ intel_driver_open(intel_driver_t *intel, cl_context_prop props)
#if defined(HAS_GBM) && defined(HAS_EGL)
if (props && props->gl_type == CL_GL_EGL_DISPLAY) {
+ assert(props->egl_display);
intel->gbm = gbm_create_device(intel->fd);
if (intel->gbm == NULL) {
printf("GBM device create failed.\n");
diff --git a/src/x11/gbm_dri2_x11_platform.c b/src/x11/gbm_dri2_x11_platform.c
index 56b2467..481f407 100644
--- a/src/x11/gbm_dri2_x11_platform.c
+++ b/src/x11/gbm_dri2_x11_platform.c
@@ -1,11 +1,93 @@
+#include <string.h>
#include "GL/gl.h" /* dri_interface need gl types definitions. */
#include "GL/internal/dri_interface.h"
#include "gbm_deps/gbm_driint.h"
#include "gbm_deps/gbmint.h"
#include "dricommon.h"
-/* image_lookup_extension is from egl_dri2.c. */
-extern const __DRIimageLookupExtension image_lookup_extension;
+typedef struct EGLDisplay _EGLDisplay;
+typedef struct EGLDriver _EGLDriver;
+/* XXX should check whether we support pthread.*/
+typedef pthread_mutex_t _EGLMutex;
+
+enum _egl_platform_type {
+ _EGL_PLATFORM_WINDOWS,
+ _EGL_PLATFORM_X11,
+ _EGL_PLATFORM_WAYLAND,
+ _EGL_PLATFORM_DRM,
+ _EGL_PLATFORM_FBDEV,
+ _EGL_PLATFORM_NULL,
+ _EGL_PLATFORM_ANDROID,
+
+ _EGL_NUM_PLATFORMS,
+ _EGL_INVALID_PLATFORM = -1
+};
+typedef enum _egl_platform_type _EGLPlatformType;
+typedef unsigned int EGLBoolean;
+typedef int32_t EGLint;
+
+struct _hack_egl_display
+{
+ /* used to link displays */
+ _EGLDisplay *Next;
+
+ _EGLMutex Mutex;
+
+ _EGLPlatformType Platform; /**< The type of the platform display */
+ void *PlatformDisplay; /**< A pointer to the platform display */
+
+ _EGLDriver *Driver; /**< Matched driver of the display */
+
+ EGLBoolean Initialized; /**< True if the display is initialized */
+
+ /* options that affect how the driver initializes the display */
+ struct {
+ EGLBoolean TestOnly; /**< Driver should not set fields when true */
+ EGLBoolean UseFallback; /**< Use fallback driver (sw or less features) */
+ } Options;
+
+ /* these fields are set by the driver during init */
+ void *DriverData; /**< Driver private data */
+ EGLint VersionMajor; /**< EGL major version */
+ EGLint VersionMinor; /**< EGL minor version */
+ EGLint ClientAPIs; /**< Bitmask of APIs supported (EGL_xxx_BIT) */
+};
+
+struct _hack_dri2_egl_display
+{
+ int dri2_major;
+ int dri2_minor;
+ __DRIscreen *dri_screen;
+ int own_dri_screen;
+ const __DRIconfig **driver_configs;
+ void *driver;
+ __DRIcoreExtension *core;
+ __DRIdri2Extension *dri2;
+ __DRIswrastExtension *swrast;
+ __DRI2flushExtension *flush;
+ __DRItexBufferExtension *tex_buffer;
+ __DRIimageExtension *image;
+ __DRIrobustnessExtension *robustness;
+ __DRI2configQueryExtension *config;
+ int fd;
+
+ int own_device;
+ int swap_available;
+ int invalidate_available;
+ int min_swap_interval;
+ int max_swap_interval;
+ int default_swap_interval;
+ struct gbm_dri_device *gbm_dri;
+
+ char *device_name;
+ char *driver_name;
+
+ __DRIdri2LoaderExtension dri2_loader_extension;
+ __DRIswrastLoaderExtension swrast_loader_extension;
+ const __DRIextension *extensions[4];
+};
+
+static __DRIimageLookupExtension *image_lookup_extension;
/* We are use DRI2 x11 platform, and by default, gbm doesn't register
* a valid image extension, and actually, it doesn't know how to register
@@ -13,8 +95,21 @@ extern const __DRIimageLookupExtension image_lookup_extension;
void cl_gbm_set_image_extension(struct gbm_device *gbm, void *display)
{
struct gbm_dri_device *gbm_dri = gbm_dri_device(gbm);
- if (gbm_dri->lookup_image == NULL) {
- gbm_dri->lookup_image = image_lookup_extension.lookupEGLImage;
+ struct _hack_egl_display *egl_dpy = (struct _hack_egl_display*)display;
+ struct _hack_dri2_egl_display *dri2_dpy = (struct _hack_dri2_egl_display*)egl_dpy->DriverData;
+ int i;
+
+ if (gbm_dri->lookup_image == NULL
+ && egl_dpy->Platform == _EGL_PLATFORM_X11) {
+ for(i = 0; i < 4; i++)
+ if (dri2_dpy->extensions[i]
+ && ((strncmp(dri2_dpy->extensions[i]->name,
+ __DRI_IMAGE_LOOKUP,
+ sizeof(__DRI_IMAGE_LOOKUP))) == 0))
+ break;
+ if (i >= 4) return;
+ image_lookup_extension = (__DRIimageLookupExtension*)dri2_dpy->extensions[i];
+ gbm_dri->lookup_image = image_lookup_extension->lookupEGLImage;
gbm_dri->lookup_user_data = display;
}
}
diff --git a/utests/compiler_fill_gl_image.cpp b/utests/compiler_fill_gl_image.cpp
index b070b8f..437fcf4 100644
--- a/utests/compiler_fill_gl_image.cpp
+++ b/utests/compiler_fill_gl_image.cpp
@@ -33,6 +33,10 @@ static void compiler_fill_gl_image(void)
uint32_t *resultColor;
GLuint tex;
+ if (eglContext == EGL_NO_CONTEXT) {
+ fprintf(stderr, "There is no valid egl context. Ignore this case.\n");
+ return;
+ }
// Setup kernel and images
glGenTextures(1, &tex);
glBindTexture(GL_TEXTURE_2D, tex);
diff --git a/utests/utest_helper.cpp b/utests/utest_helper.cpp
index 941b5f9..79ba30c 100644
--- a/utests/utest_helper.cpp
+++ b/utests/utest_helper.cpp
@@ -88,7 +88,8 @@ bool init_egl_window(int width, int height) {
EGLConfig ecfg;
EGLint numConfig;
-
+
+ eglContext = EGL_NO_CONTEXT;
xDisplay = XOpenDisplay(NULL);
if (xDisplay == NULL) {
fprintf(stderr, "Failed to open DISPLAY.\n");
@@ -343,15 +344,17 @@ cl_ocl_init(void)
#ifdef HAS_EGL
if (hasGLExt) {
- init_egl_window(EGL_WINDOW_WIDTH, EGL_WINDOW_HEIGHT);
+ int i = 0;
props = new cl_context_properties[7];
- props[0] = CL_CONTEXT_PLATFORM;
- props[1] = (cl_context_properties)platform;
- props[2] = CL_EGL_DISPLAY_KHR;
- props[3] = (cl_context_properties)eglGetCurrentDisplay();
- props[4] = CL_GL_CONTEXT_KHR;
- props[5] = (cl_context_properties)eglGetCurrentContext();
- props[6] = 0;
+ props[i++] = CL_CONTEXT_PLATFORM;
+ props[i++] = (cl_context_properties)platform;
+ if (init_egl_window(EGL_WINDOW_WIDTH, EGL_WINDOW_HEIGHT)) {
+ props[i++] = CL_EGL_DISPLAY_KHR;
+ props[i++] = (cl_context_properties)eglGetCurrentDisplay();
+ props[i++] = CL_GL_CONTEXT_KHR;
+ props[i++] = (cl_context_properties)eglGetCurrentContext();
+ }
+ props[i++] = 0;
}
#endif
/* Now create a context */
--
1.7.9.5
Reply to: