Re: [waffle] [PATCH 7/7] nacl: add implementation for waffle_dl_sym

[Emil Velikov]

On 6 February 2015 at 19:44, Chad Versace chad.vers...@intel.com wrote:
...

 The way that Waffle's Linux and Mac platforms work today, the user doesn't
 have to call waffle_dl_can_open(). When the user calls waffle_dl_sym(),
 Waffle internally calls dlopen() if the user hasn't called waffle_dl_sym()
 on that library before.

Thank you for the correction Chad.

I have to respectfully disagree for one bit - waffle_dl_sym does not
directly call waffle_dl_can_open. The former does trigger an
waffle_error as an invalid dl is selected, while the latter should
not*.

Currently Tapani directly executes nacl_dl_can_open within nacl_dl_sym
- thus no error is emitted for the latter. Imho he can resolve this by
following following cgl/linux example.

Cheers
Emil

[*] Upon closer look it seems that cgl does set a waffle_error on
waffle_dl_can_open. I will send out a patch later on today.
___
waffle mailing list
waffle@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/waffle

Re: [waffle] [PATCH 7/7] nacl: add implementation for waffle_dl_sym

[Tapani]

On 02/06/2015 09:44 PM, Chad Versace wrote:

On 02/05/2015 09:05 AM, Emil Velikov wrote:

On 5 February 2015 at 13:12, Tapani tapani.pa...@intel.com wrote:

On 02/03/2015 07:09 PM, Emil Velikov wrote:

On 23 January 2015 at 07:59, Tapani Pälli tapani.pa...@intel.com wrote:

...

+
+uint32_t len = strlen(src) + strlen(prefix);

So the function name changes from glHamSandwitch to GLES2HamSandwitch ?
In that case you'll need to subtract 2 from the len above.

I believe only 1 because of the additional string terminator, but I consider
above cleaner than playing the +/- game (?)


You're correct - I've completely forgot about \0, also I agree with
your argument.

...

@@ -59,7 +97,28 @@ nacl_platform_dl_sym(struct wcore_platform *wc_self,
int32_t waffle_dl,
const char *name)
   {
-return NULL;
+struct nacl_platform *self = nacl_platform(wc_self);
+char *nacl_name = NULL;
+void *func = NULL;
+
+if (!self-gl_dl)
+if (!nacl_platform_dl_can_open(wc_self, waffle_dl))

One should not be doing this - it's the user's responsibility to call
dl_can_open prior to dl_sym.


Documentation does not seem to indicate mandatory call to dl_can_open(), it
just suggests that one can test if a dll can be opened this way. AFAICS one
can just start to dlsym() the required functions?


I believe the lack of documentation is due to historical reasons.

Imho it's a picky subject what is to be done, if one can do XX
context, but does not have (direct) library to dlsym the symbols from.
I believe I've annoyed Chad enough on the topic so I won't pick it up
again :-)

I would personally opt to updating the documentation, as afaics none
of the current implementations calls dl_can_open within dl_sym.
Obviously the final decision is not mine to make, so this is just my 2c.

The way that Waffle's Linux and Mac platforms work today, the user doesn't
have to call waffle_dl_can_open(). When the user calls waffle_dl_sym(),
Waffle internally calls dlopen() if the user hasn't called waffle_dl_sym()
on that library before.

OK, thanks for this clarification.


Tapani, when I reviewed it earlier, I
misundertood how you were internally using nacl_platform_dl_can_open() inside
nacl_platform_dl_sym(). But, now I think the way you're doing it is correct.
I retract my earlier review comment.

However, after looking more closely at the patch, it looks like
nacl_platform_dl_can_open() re-opens the library with dlopen()
each time the user calls waffle_dl_can_open().


oops yes, it's missing the check, I will fix this.





___
waffle mailing list
waffle@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/waffle


___
waffle mailing list
waffle@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/waffle

Re: [waffle] [PATCH 7/7] nacl: add implementation for waffle_dl_sym

[Chad Versace]

On 02/05/2015 09:05 AM, Emil Velikov wrote:
 On 5 February 2015 at 13:12, Tapani tapani.pa...@intel.com wrote:
 On 02/03/2015 07:09 PM, Emil Velikov wrote:

 On 23 January 2015 at 07:59, Tapani Pälli tapani.pa...@intel.com wrote:
 ...
 +
 +uint32_t len = strlen(src) + strlen(prefix);

 So the function name changes from glHamSandwitch to GLES2HamSandwitch ?
 In that case you'll need to subtract 2 from the len above.

 I believe only 1 because of the additional string terminator, but I consider
 above cleaner than playing the +/- game (?)

 You're correct - I've completely forgot about \0, also I agree with
 your argument.
 
 ...
 @@ -59,7 +97,28 @@ nacl_platform_dl_sym(struct wcore_platform *wc_self,
int32_t waffle_dl,
const char *name)
   {
 -return NULL;
 +struct nacl_platform *self = nacl_platform(wc_self);
 +char *nacl_name = NULL;
 +void *func = NULL;
 +
 +if (!self-gl_dl)
 +if (!nacl_platform_dl_can_open(wc_self, waffle_dl))

 One should not be doing this - it's the user's responsibility to call
 dl_can_open prior to dl_sym.


 Documentation does not seem to indicate mandatory call to dl_can_open(), it
 just suggests that one can test if a dll can be opened this way. AFAICS one
 can just start to dlsym() the required functions?

 I believe the lack of documentation is due to historical reasons.
 
 Imho it's a picky subject what is to be done, if one can do XX
 context, but does not have (direct) library to dlsym the symbols from.
 I believe I've annoyed Chad enough on the topic so I won't pick it up
 again :-)
 
 I would personally opt to updating the documentation, as afaics none
 of the current implementations calls dl_can_open within dl_sym.
 Obviously the final decision is not mine to make, so this is just my 2c.

The way that Waffle's Linux and Mac platforms work today, the user doesn't
have to call waffle_dl_can_open(). When the user calls waffle_dl_sym(),
Waffle internally calls dlopen() if the user hasn't called waffle_dl_sym()
on that library before.

Tapani, when I reviewed it earlier, I
misundertood how you were internally using nacl_platform_dl_can_open() inside
nacl_platform_dl_sym(). But, now I think the way you're doing it is correct.
I retract my earlier review comment.

However, after looking more closely at the patch, it looks like
nacl_platform_dl_can_open() re-opens the library with dlopen()
each time the user calls waffle_dl_can_open().




signature.asc
Description: OpenPGP digital signature
___
waffle mailing list
waffle@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/waffle

Re: [waffle] [PATCH 7/7] nacl: add implementation for waffle_dl_sym

[Tapani]

On 02/03/2015 07:09 PM, Emil Velikov wrote:

On 23 January 2015 at 07:59, Tapani Pälli tapani.pa...@intel.com wrote:

Signed-off-by: Tapani Pälli tapani.pa...@intel.com

You might want to model dl handling similar to cgl_dl - both support
only single waffle_dl.
As such I won't pick on the diffs between the two.


Thanks, I'll take a look at this.


+// Construct a string that maps GL function to NaCl function
+// by concating given prefix and function name tail from 'src'.
+static char *
+nacl_prefix(const char *src, const char *prefix)
+{
+if (strncmp(src, gl, 2) != 0)
+return NULL;

wcore_error ?


Yep can add this, I was not sure if it is and error if symbol is not found.


+
+uint32_t len = strlen(src) + strlen(prefix);

So the function name changes from glHamSandwitch to GLES2HamSandwitch ?
In that case you'll need to subtract 2 from the len above.
I believe only 1 because of the additional string terminator, but I 
consider above cleaner than playing the +/- game (?)



+
+char *dst = calloc(len, 1);

Using wcore_calloc will set a nice waffle_error in case this fails :-)

Nice, will start to use that.


[...]

@@ -59,7 +97,28 @@ nacl_platform_dl_sym(struct wcore_platform *wc_self,
   int32_t waffle_dl,
   const char *name)
  {
-return NULL;
+struct nacl_platform *self = nacl_platform(wc_self);
+char *nacl_name = NULL;
+void *func = NULL;
+
+if (!self-gl_dl)
+if (!nacl_platform_dl_can_open(wc_self, waffle_dl))

One should not be doing this - it's the user's responsibility to call
dl_can_open prior to dl_sym.


Documentation does not seem to indicate mandatory call to dl_can_open(), 
it just suggests that one can test if a dll can be opened this way. 
AFAICS one can just start to dlsym() the required functions?



+return false;
+
+switch (waffle_dl) {
+case WAFFLE_DL_OPENGL_ES2:
+nacl_name = nacl_prefix(name, GLES2);
+break;
+}
+

Just drop the switch for now ?

Can do. Thanks for the reviews Emil!


Cheers,
Emil



// Tapani

___
waffle mailing list
waffle@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/waffle

Re: [waffle] [PATCH 7/7] nacl: add implementation for waffle_dl_sym

[Emil Velikov]

On 5 February 2015 at 13:12, Tapani tapani.pa...@intel.com wrote:
 On 02/03/2015 07:09 PM, Emil Velikov wrote:

 On 23 January 2015 at 07:59, Tapani Pälli tapani.pa...@intel.com wrote:
...
 +
 +uint32_t len = strlen(src) + strlen(prefix);

 So the function name changes from glHamSandwitch to GLES2HamSandwitch ?
 In that case you'll need to subtract 2 from the len above.

 I believe only 1 because of the additional string terminator, but I consider
 above cleaner than playing the +/- game (?)

You're correct - I've completely forgot about \0, also I agree with
your argument.

...
 @@ -59,7 +97,28 @@ nacl_platform_dl_sym(struct wcore_platform *wc_self,
int32_t waffle_dl,
const char *name)
   {
 -return NULL;
 +struct nacl_platform *self = nacl_platform(wc_self);
 +char *nacl_name = NULL;
 +void *func = NULL;
 +
 +if (!self-gl_dl)
 +if (!nacl_platform_dl_can_open(wc_self, waffle_dl))

 One should not be doing this - it's the user's responsibility to call
 dl_can_open prior to dl_sym.


 Documentation does not seem to indicate mandatory call to dl_can_open(), it
 just suggests that one can test if a dll can be opened this way. AFAICS one
 can just start to dlsym() the required functions?

I believe the lack of documentation is due to historical reasons.

Imho it's a picky subject what is to be done, if one can do XX
context, but does not have (direct) library to dlsym the symbols from.
I believe I've annoyed Chad enough on the topic so I won't pick it up
again :-)

I would personally opt to updating the documentation, as afaics none
of the current implementations calls dl_can_open within dl_sym.
Obviously the final decision is not mine to make, so this is just my 2c.

 +return false;
 +
 +switch (waffle_dl) {
 +case WAFFLE_DL_OPENGL_ES2:
 +nacl_name = nacl_prefix(name, GLES2);
 +break;
 +}
 +

 Just drop the switch for now ?

 Can do. Thanks for the reviews Emil!

You're welcome :-)

-Emil
___
waffle mailing list
waffle@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/waffle

Re: [waffle] [PATCH 7/7] nacl: add implementation for waffle_dl_sym

[Emil Velikov]

On 23 January 2015 at 07:59, Tapani Pälli tapani.pa...@intel.com wrote:
 Signed-off-by: Tapani Pälli tapani.pa...@intel.com

You might want to model dl handling similar to cgl_dl - both support
only single waffle_dl.
As such I won't pick on the diffs between the two.

 +// Construct a string that maps GL function to NaCl function
 +// by concating given prefix and function name tail from 'src'.
 +static char *
 +nacl_prefix(const char *src, const char *prefix)
 +{
 +if (strncmp(src, gl, 2) != 0)
 +return NULL;
wcore_error ?

 +
 +uint32_t len = strlen(src) + strlen(prefix);
So the function name changes from glHamSandwitch to GLES2HamSandwitch ?
In that case you'll need to subtract 2 from the len above.

 +
 +char *dst = calloc(len, 1);
Using wcore_calloc will set a nice waffle_error in case this fails :-)

[...]
 @@ -59,7 +97,28 @@ nacl_platform_dl_sym(struct wcore_platform *wc_self,
   int32_t waffle_dl,
   const char *name)
  {
 -return NULL;
 +struct nacl_platform *self = nacl_platform(wc_self);
 +char *nacl_name = NULL;
 +void *func = NULL;
 +
 +if (!self-gl_dl)
 +if (!nacl_platform_dl_can_open(wc_self, waffle_dl))
One should not be doing this - it's the user's responsibility to call
dl_can_open prior to dl_sym.

 +return false;
 +
 +switch (waffle_dl) {
 +case WAFFLE_DL_OPENGL_ES2:
 +nacl_name = nacl_prefix(name, GLES2);
 +break;
 +}
 +
Just drop the switch for now ?

Cheers,
Emil
___
waffle mailing list
waffle@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/waffle