[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#712880: [Beignet] *** SPAM LEVEL 5.929 *** beignet: libEGL ABI abuse (getting at symbols not intended to be public)



OK, this version works well.


On 07/05/2013 05:29 PM, Zhigang Gong wrote:
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

      

_______________________________________________
Beignet mailing list
Beignet@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/beignet


Reply to: