On Mon, Feb 26, 2018 at 6:12 PM, Francisco Jerez <curroje...@riseup.net> wrote:
> Rob Clark <robdcl...@gmail.com> writes:
>
>> On Sun, Feb 25, 2018 at 3:00 PM, Francisco Jerez <curroje...@riseup.net>
>> wrote:
>>> Seems like a serious hack to me to work around broken linking... IMO we
>>> should just fix the linking issue. The symbols for the various GLSL
>>> types need to be linked with the proper binding and visibility -- I
>>> assume that the cause of your problem is that people are making
>>> assumptions about the equality of GLSL types based on their memory
>>> addresses *and* marking the symbols as hidden *and* passing pointers to
>>> GLSL types across shared objects? That sounds like a recipe for
>>> disaster.
>>
>> tbh, maybe hack, or I think more likely, maybe a good idea.. I'm not
>> terribly sold on the idea of dynamically loading pipe driver and
>> linking a lot of shared code into N different pipe_${driver}.so on
>> disk, since the # of drivers seems to be greater than # of state
>> trackers.. not to mention multiple copies of shared gallium code in
>> memory due to being statically linked into both state tracker and
>> driver.
>>
>
> It would probably make sense to link that shared code dynamically into
> the pipe-loader modules in order to avoid the duplication you're talking
> about (e.g. by building them as a meson shared_module() instead of a
> shared_library() in order to allow the common symbols to be undefined in
> the pipe-loader module which will then be pulled in by the state
> tracker) -- Actually that's likely to fix your problem too.
yes, for pipe_loader_dynamic case, this is the only way I can see for
it to work (although I think we still want to link statically in the
pipe_loader_static case). Well that, or making the glsl_types symbols
not -Bsymbolic.
The potential problem here is, what if an app links in both (for ex.)
CL + GL, but somehow from different mesa versions. I suppose that
might be a corner case, but it would fall over badly w/ nir/glsl_types
in a .so. But I think the way things are packaged in most distro's, a
user could upgrade specifically the mesa-gl package but not the
mesa-cl package. (Also, we don't really want to export glsl/nir
symbols lest someone mistake them for a stable ABI.)
I guess I am ok with the approach of (a) adding support for "mega-cl"
using piple_loader_static plus (b) adding a libnir.so for the dynamic
case, and letting users of pipe_loader_dynamic keep both pieces in the
broken corner cases.
>> That said, glsl_type's defn does seem to expect to only exist once in
>> a process, ie. == is ptr comparison and when you link nir/glsl_types
>> into both state tracker and driver, glsl_type ptrs are getting passed
>> across that boundary. I'm not really sure that is worth fixing (ie.
>> why should it exist twice in a process in the first place?)
>>
>
> *Shrug* It's kind of a micro-optimization with nasty side effects
> (namely that you cannot pass GLSL IR or NIR across shared object
> boundaries without things exploding) and dubious payoff, it would
> probably be reasonable to simply drop the micro-optimization, but that
> wasn't the solution I was suggesting (but rather, making the
> optimization work by exporting the symbols GLSL type equality cares
> about with the right linkage).
well, I think it is long past too late to try to solve that. If it
were only in c++ code we could overload == but it is a mix of c++ and
c, and tracking down all the places that do pointer comparisons seems
like it would be tricky to avoid missing some and having subtle bugs
as a result.
>> Maybe there are some linker tricks to solve this, idk.. that is a bit
>> outside my area of expertise.
>
> More than adding new linker tricks I think you just need to make sure
> that existing linker tricks we're doing right now ('-fvisibility=hidden'
> and various linker scripts) don't break the build by forcing common
> symbols to be local, along the lines of:
>
> | diff --git a/src/compiler/glsl_types.h b/src/compiler/glsl_types.h
> | index ab0b2637649..9d8a5ea341c 100644
> | --- a/src/compiler/glsl_types.h
> | +++ b/src/compiler/glsl_types.h
> | @@ -243,7 +243,7 @@ public:
> | /*@{*/
> | #undef DECL_TYPE
> | #define DECL_TYPE(NAME, ...) \
> | - static const glsl_type *const NAME##_type;
> | + static const glsl_type *const NAME##_type
> __attribute__((visibility("default")));
> | #undef STRUCT_TYPE
> | #define STRUCT_TYPE(NAME) \
> | static const glsl_type *const struct_##NAME##_type;
> | diff --git a/src/gallium/targets/opencl/opencl.sym
> b/src/gallium/targets/opencl/opencl.sym
> | index 9fcc57692b8..fadc2eb6c1d 100644
> | --- a/src/gallium/targets/opencl/opencl.sym
> | +++ b/src/gallium/targets/opencl/opencl.sym
> | @@ -2,6 +2,10 @@
> | global:
> | cl*;
> | opencl_dri_*;
> | +
> | + extern "C++" {
> | + glsl_type::*;
> | + };
> | local:
> | *;
> | };
> | diff --git a/src/gallium/targets/pipe-loader/pipe.sym
> b/src/gallium/targets/pipe-loader/pipe.sym
> | index 605cb83d802..801b0f44d56 100644
> | --- a/src/gallium/targets/pipe-loader/pipe.sym
> | +++ b/src/gallium/targets/pipe-loader/pipe.sym
> | @@ -7,6 +7,10 @@
> | # due to LLVM being initialized multiple times.
> | radeon_drm_winsys_create;
> | amdgpu_winsys_create;
> | +
> | + extern "C++" {
> | + glsl_type::*;
> | + };
> | local:
> | *;
> | };
>
iiuc, this would have the same corner case of moving nir/glsl_types to
a separate .so when mixing state tracker versions in a process.
>> To me, the mega-driver approach worked well for gl, and many of the
>> other state trackers are already using libpipe_loader_static.. why not
>> just fix the problems with that approach and abandon
>> libpipe_loader_dynamic? This seems like it would work out better for
>> memory footprint since pages for drivers not used simply wouldn't get
>> paged in. And you aren't ending up with two copies of libgallium or
>> libnir code in memory.
>>
>
> The dynamic pipe loader would give you better disk and memory footprint
> than the static one assuming that the problem with linking shared code
> statically into each pipe-loader module was fixed, because that would
> allow the pipe driver code to be shared in memory and disk among
> different state trackers, but I guess that the benefit is slight at this
> point, I understand your reluctance to fix it ;).
>
I guess I haven't measured, but I wonder if this is even true. The
cost increase is duplicating pipe_driver code in each state tracker
(but I guess there are only 3 or 4 that typically end up on users
filesys, and of those only mesa/st really needs to link in *all* the
drivers, clover and the video state trackers are only supported by a
few drivers each), but is that offset by de-duplicating shared code
(gallium/aux, libnir, etc)?
I did have a thought earlier today. Since we have glvnd, and
opencl-icd (and I guess vdpau and vaapi have a sort of loader), where
the state tracker isn't directly exporting the API symbols but rather
just a jump-table used by the icd .so, we could actually perhaps do a
mega-mega driver where all the state trackers and all the drivers are
linked into a single .so.. That would reduce the disk and memory
footprint the most. (Well, ok, it would cost maybe some virtual
address space, but pages associated w/ unused state trackers wouldn't
be demand-paged in, so meh?)
BR,
-R
>> BR,
>> -R
>>[...]
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
