Bug#712880: [Beignet] *** SPAM LEVEL 5.929 *** beignet: libEGL ABI abuse (getting at symbols not intended to be public)
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
>From a119c0185c23a6a124fd5502d3128190f80c67dc 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] 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.
Signed-off-by: Zhigang Gong <zhigang.gong@linux.intel.com>
---
src/x11/gbm_dri2_x11_platform.c | 103 +++++++++++++++++++++++++++++++++++++--
1 file changed, 99 insertions(+), 4 deletions(-)
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;
}
}
--
1.7.9.5
Reply to: