libc constants for js-ctypes (Chrome Workers)

RESOLVED FIXED in mozilla15

Status

()

Core
js-ctypes
--
enhancement
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

unspecified
mozilla15
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 10 obsolete attachments)

11.09 KB, patch
Details | Diff | Splinter Review
Not quite sure in which component to place this bug.

Now that we (almost) have errno support in js-ctypes (see bug 684017), we still need to be able to compare error numbers with their actual definition in errno.h. As these values are defined as macros and are not portable among OSes, this requires some C/C++.

More generally, we need to export to JavaScript all the libc constants, including constants for |open|, |seek|, etc.

I will open another bug for (less portable) Windows-specific constants.
Created attachment 609840 [details] [diff] [review]
Exporting libc constants to JS

Attaching a first draft. This module exports a global object |OS.Constants.libc|, with many (all?) of the important libc constants. I envision that this module can be later extended with additional constants as needed.
Attachment #609840 - Flags: feedback?(mh+mozilla)
Attachment #609840 - Flags: feedback?(jorendorff)
Assignee: nobody → dteller
Comment on attachment 609840 [details] [diff] [review]
Exporting libc constants to JS

Review of attachment 609840 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/osfile/Makefile.in
@@ +55,5 @@
>  
>  EXPORTS_mozilla = OSFile.h \
>     $(NULL)
>  
> +CPPSRCS = OSFileConstants.cpp \

Please put that on a separate line.

::: toolkit/components/osfile/OSFileConstants.cpp
@@ +25,5 @@
> +  /**
> +   * JS value of the property. Any JS value will do.
> +   */
> +  jsval value;
> +  unsigned flags;

Considering the flags are always the same, you don't need this.

@@ +179,5 @@
> +  INT_CONSTANT(EROFS),
> +  INT_CONSTANT(ESPIPE),
> +  INT_CONSTANT(ETIMEDOUT),
> +  INT_CONSTANT(ETXTBSY),
> +  INT_CONSTANT(EWOULDBLOCK),

I don't think we want to be that exhaustive in the exposed constants. (esp. since each and every one of them adds a string and a relocation)
Attachment #609840 - Flags: feedback?(mh+mozilla) → feedback-
(In reply to Mike Hommey [:glandium] from comment #2)
> Comment on attachment 609840 [details] [diff] [review]
> Exporting libc constants to JS
> 
> Review of attachment 609840 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/osfile/Makefile.in
> @@ +55,5 @@
> >  
> >  EXPORTS_mozilla = OSFile.h \
> >     $(NULL)
> >  
> > +CPPSRCS = OSFileConstants.cpp \
> 
> Please put that on a separate line.
> 
> ::: toolkit/components/osfile/OSFileConstants.cpp
> @@ +25,5 @@
> > +  /**
> > +   * JS value of the property. Any JS value will do.
> > +   */
> > +  jsval value;
> > +  unsigned flags;
> 
> Considering the flags are always the same, you don't need this.

I initially intended to add some variety, but you are right.

> @@ +179,5 @@
> > +  INT_CONSTANT(EROFS),
> > +  INT_CONSTANT(ESPIPE),
> > +  INT_CONSTANT(ETIMEDOUT),
> > +  INT_CONSTANT(ETXTBSY),
> > +  INT_CONSTANT(EWOULDBLOCK),
> 
> I don't think we want to be that exhaustive in the exposed constants. (esp.
> since each and every one of them adds a string and a relocation)

My protocol was the following: read the manpage of every function I was exporting, and add all the errors that appear somewhere in that manpage. Now, if you think that these constants are useless, I can remove them, but do you have a good idea on which list would be reasonable?
(In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> My protocol was the following: read the manpage of every function I was
> exporting, and add all the errors that appear somewhere in that manpage.
> Now, if you think that these constants are useless, I can remove them, but
> do you have a good idea on which list would be reasonable?

I think it would be reasonable to only define those that are useful to us. If anyone requires more, they can 1. file a bug and/or 2. define the constant themselves.
Comment on attachment 609840 [details] [diff] [review]
Exporting libc constants to JS

Fine with me (but of course don't take my feedback as overriding glandium's).
Attachment #609840 - Flags: feedback?(jorendorff) → feedback+
Created attachment 612554 [details] [diff] [review]
Exporting libc constants to JS

Reduced the number of symbols involved to what seems a "reasonable" subset of libc (basically, the constants used by the functions I am using in OS.File tests atm). Does this sound like a reasonable set of constants?
Attachment #609840 - Attachment is obsolete: true
Attachment #612554 - Flags: review?(taras.mozilla)
Created attachment 612555 [details] [diff] [review]
Building and exporting the module
Attachment #612555 - Flags: review?(taras.mozilla)

Comment 8

5 years ago
Comment on attachment 612554 [details] [diff] [review]
Exporting libc constants to JS

r+ for the general idea of compiling a string->int table.



SimplePropertySpec gLibcProperties[]  should be static or in anon name


I'm also not a good reviewer for nsIExporting interesting new bits.

If i was I would say:
I think IOSFileService.error_codes should be implemented by returning a JSObject with a lazy getter so when now does nsIOSFileService.error_codes.O_RDONLY, .error_codes returns an object that implements a lazy getter (via JS Resolve hook?) that then looks up the "O_RDONLY" property in a table or a generated switch statement(table is probably better, since that makes it easier to support enumeration).space

Perhaps one of khuey/bent/jst/bholley etc be a better reviewer?
Attachment #612554 - Flags: review?(taras.mozilla) → review-

Comment 9

5 years ago
Comment on attachment 612555 [details] [diff] [review]
Building and exporting the module

Kyle got more makefile-foo than me
Attachment #612555 - Flags: review?(taras.mozilla) → review?(khuey)

Comment 10

5 years ago
David, I nominate dougt as a toolkit reviewer for file api. I think he implemented part of the nsIFile* stuff.

It is probably wise to disregard my lazyness suggestion in the above review.
Comment on attachment 612555 [details] [diff] [review]
Building and exporting the module

Review of attachment 612555 [details] [diff] [review]:
-----------------------------------------------------------------

I don't really understand what you're trying to do here.  Why is there an XPCOM service involved at all?

Having dom/ include stuff from toolkit/ seems kind of tacky too.  Sticking this in dom/system seems reasonable to me.
Attachment #612555 - Flags: review?(khuey) → review-
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #11)
> Comment on attachment 612555 [details] [diff] [review]
> Building and exporting the module
> 
> Review of attachment 612555 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't really understand what you're trying to do here.  Why is there an
> XPCOM service involved at all?

Objective: export libc constants so that libraries based on js-ctypes can use them. This cannot be done in pure JS because constants differ between platforms, or from js-ctypes, because these constants are actually #define.

Well, the patch in dom/ ensures that chrome workers have access to this object. The xpcom service ensures that main thread can have access to it, too. I could also have automatically patched the global scope of chrome main thread contexts, but this less invasive technique was recommended to me a few months ago by someone on the jsapi team (can't remember who).

> Having dom/ include stuff from toolkit/ seems kind of tacky too.  Sticking
> this in dom/system seems reasonable to me.

Ok.
(In reply to Taras Glek (:taras) from comment #8)
> Comment on attachment 612554 [details] [diff] [review]
> Exporting libc constants to JS
> 
> r+ for the general idea of compiling a string->int table.
> 
> 
> 
> SimplePropertySpec gLibcProperties[]  should be static or in anon name

ok.

> I'm also not a good reviewer for nsIExporting interesting new bits.
> 
> If i was I would say:
> I think IOSFileService.error_codes should be implemented by returning a
> JSObject with a lazy getter so when now does
> nsIOSFileService.error_codes.O_RDONLY, .error_codes returns an object that
> implements a lazy getter (via JS Resolve hook?) that then looks up the
> "O_RDONLY" property in a table or a generated switch statement(table is
> probably better, since that makes it easier to support enumeration).space

This sounds feasible, but I am not sure what this change would bring. If we don't use a |switch| statement, this essentially means that we handle atomization and the hash table manually instead of letting JSAPI handling it. We might possibly save a little memory, but I fear that we will lose some JIT inlining opportunities, too. Plus code will be more complicated.


> 
> Perhaps one of khuey/bent/jst/bholley etc be a better reviewer?
Created attachment 614762 [details] [diff] [review]
Exporting libc constants to JS

Moved to dom/system, as discussed, and removed the xpcom component for the time being. This version also introduces some of the Windows constants.
Attachment #612554 - Attachment is obsolete: true
Attachment #612555 - Attachment is obsolete: true
Attachment #614762 - Flags: review?(khuey)
Created attachment 617430 [details] [diff] [review]
Exporting libc constants to JS

Same patch, but without Windows stuff.
Attachment #614762 - Attachment is obsolete: true
Attachment #614762 - Flags: review?(khuey)
Attachment #617430 - Flags: review?(khuey)
Comment on attachment 617430 [details] [diff] [review]
Exporting libc constants to JS

Review of attachment 617430 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/Makefile.in
@@ +86,1 @@
>    $(NULL)

Please export OSFileConstants with namespacing.

e.g.

EXPORTS_NAMESPACES = mozilla

EXPORTS_mozilla = OSFileConstants.h

::: dom/system/OSFileConstants.cpp
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +
> +#include "fcntl.h"

Nit: one newline between the license block and the #include.

@@ +34,5 @@
> +  /**
> +   * JS value of the property. Any JS value will do.
> +   */
> +  jsval value;
> +} SimplePropertySpec;

Lets just reuse ConstantSpec from dom/bindings/Utils.h.

@@ +53,5 @@
> + */
> +#define PROP_END { NULL, JSVAL_VOID }
> +
> +
> +// Define placeholders for Android

Really?  Android is such a POS sometimes.

Why do we want to define these to wrong values rather than leaving them out entirely?

@@ +238,5 @@
> +                           NULL, NULL,
> +                           flags) ) {
> +      return false;
> +    }
> +  }

Conveniently, there's a DefineConstants in dom/bindings/Utils.cpp.  Lets expose that in Utils.h and use it here.

::: dom/system/OSFileConstants.h
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +
> +#ifndef mozilla_osfile_osfileconstants_h__

Nit: one newline between the license block and the ifndef.

@@ +5,5 @@
> +
> +#ifndef mozilla_osfile_osfileconstants_h__
> +#define mozilla_osfile_osfileconstants_h__
> +
> +#include "jsapi.h"

Please include jspubtd.h instead of jsapi.h here.

@@ +8,5 @@
> +
> +#include "jsapi.h"
> +
> +namespace mozilla {
> +namespace osfile {

Lets drop the second level of namespacing here, and just have mozilla::DefineOSFilePlatformConstants.

::: dom/workers/Makefile.in
@@ +59,5 @@
>  LOCAL_INCLUDES = \
>    -I$(topsrcdir)/content/base/src \
>    -I$(topsrcdir)/content/events/src \
>    -I$(topsrcdir)/dom/base \
> +  -I$(topsrcdir)/dom/system \

This should not be necessary.
Attachment #617430 - Flags: review?(khuey)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #16)
> > + */
> > +#define PROP_END { NULL, JSVAL_VOID }
> > +
> > +
> > +// Define placeholders for Android
> 
> Really?  Android is such a POS sometimes.
> 
> Why do we want to define these to wrong values rather than leaving them out
> entirely?

Well, the reasoning is the following. I suspect that future users of this library are not aware that these constants do not exist specifically on Android. As function |open| still takes an argument obtained by composing these constants (which is just ignored on Android), I suspect that users will try and compose these constants. If the constants are undefined, the result will be |undefined|, which, iirc, will cause a surprising and hard-to-read runtime error during the call to the C function.

Being myself the primary user for this function, it also saves me from having to write a distinct variant of the high-level file-opening functions specifically for Android.

> Conveniently, there's a DefineConstants in dom/bindings/Utils.cpp.  Lets
> expose that in Utils.h and use it here.

Ok, I will look at that.

> Lets drop the second level of namespacing here, and just have
> mozilla::DefineOSFilePlatformConstants.

Ok.

Thanks for the quick review.
(In reply to David Rajchenbach Teller [:Yoric] from comment #17)
> Well, the reasoning is the following. I suspect that future users of this
> library are not aware that these constants do not exist specifically on
> Android. As function |open| still takes an argument obtained by composing
> these constants (which is just ignored on Android), I suspect that users
> will try and compose these constants. If the constants are undefined, the
> result will be |undefined|, which, iirc, will cause a surprising and
> hard-to-read runtime error during the call to the C function.

Why not either define the actual values or entirely leave these constants out? They kind of look like expensive ways to write 0755 and such, which we have been happily using in JS without defining constants so far.
Well, that was when 0755 was legal in JS, wasn't it?
Now, using octal in JS raises a warning or an error in strict mode.
(In reply to David Rajchenbach Teller [:Yoric] from comment #19)
> Well, that was when 0755 was legal in JS, wasn't it?
> Now, using octal in JS raises a warning or an error in strict mode.

Then that's even more a reason *not* to put broken values on Android.
Not sure I understand. What would you suggest?
Setting real values instead of 0
You want me to put the Linux values even though they are meaningless on Android? I can do that, but what is the point?
(In reply to David Rajchenbach Teller [:Yoric] from comment #23)
> You want me to put the Linux values even though they are meaningless on
> Android? I can do that, but what is the point?

S_IRGRP and friends? meaningless on Android? They have the same values and meaning on any unix system, OSX included.
I had the impression that the Android file system does not care about these constants. Am I wrong?
Except /sdcard, which is vfat, it does care about them. Regardless, all you'd achieve with 0 on every value is to create unreadable files and directories.
Ok, I will fix this.
Created attachment 618199 [details] [diff] [review]
Exporting libc constants to JS
Attachment #617430 - Attachment is obsolete: true
Attachment #618199 - Flags: review?(khuey)
Comment on attachment 618199 [details] [diff] [review]
Exporting libc constants to JS

Review of attachment 618199 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like glandium to sign off on the Android stuff.

::: dom/system/OSFileConstants.cpp
@@ +183,5 @@
> +  if (JS_GetProperty(cx, aObject, aProperty, &val)
> +      && !JSVAL_IS_VOID(val)) {
> +    if (JSVAL_IS_OBJECT(val)) {
> +      return JSVAL_TO_OBJECT(val);
> +    } else {

}
else {

is the prevailing style here.

@@ +212,5 @@
> +  JSObject *objLibc;
> +  if (!(objLibc = GetOrCreateObjectProperty(cx, objConstants, "libc"))) {
> +    return false;
> +  }
> +  return dom::bindings::DefineConstants(cx, objLibc, gLibcProperties);

Is it intentional that we leave OS.Constants empty?

::: dom/system/OSFileConstants.h
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_osfile_osfileconstants_h__
> +#define mozilla_osfile_osfileconstants_h__

mozilla_osfileconstants_h__

::: dom/workers/WorkerScope.cpp
@@ +62,5 @@
>  #include "File.h"
>  #include "FileReaderSync.h"
>  #include "Location.h"
>  #include "Navigator.h"
> +#include "OSFileConstants.h"

mozilla/OSFileConstants.h ?
Attachment #618199 - Flags: review?(mh+mozilla)
Attachment #618199 - Flags: review?(khuey)
Attachment #618199 - Flags: review+
Created attachment 619507 [details] [diff] [review]
Exporting libc constants to JS
Attachment #618199 - Attachment is obsolete: true
Attachment #618199 - Flags: review?(mh+mozilla)
Attachment #619507 - Flags: review?(mh+mozilla)
Summary: libc constants for js-ctypes → libc constants for js-ctypes (Chrome Workers)
Blocks: 750177
Blocks: 747872
Blocks: 750178
Comment on attachment 619507 [details] [diff] [review]
Exporting libc constants to JS

Review of attachment 619507 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/OSFileConstants.cpp
@@ +50,5 @@
> +#define S_IWOTH 2
> +#define S_IWUSR 128
> +#define S_IXOTH 1
> +#define S_IXGRP 8
> +#define S_IXUSR 64

Octal values would be more obvious to the eye. (S_IRGRP is 0040, S_IRUSR 0400, S_IROTH 0004, etc.)

@@ +120,5 @@
> +  INT_CONSTANT(W_OK),
> +  INT_CONSTANT(X_OK),
> +#endif // defined(F_OK)
> +
> +  // modes (not defined on Android)

"not defined on Android" not true anymore, right?
Attachment #619507 - Flags: review?(mh+mozilla) → review+
Created attachment 619512 [details] [diff] [review]
Exporting libc constants to JS

Fixed. Thanks for the reviews!
Attachment #619507 - Attachment is obsolete: true
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86 → All
unable to find 'dom/bindings/Utils.cpp' for patching
1 out of 1 hunks FAILED -- saving rejects to file dom/bindings/Utils.cpp.rej
unable to find 'dom/bindings/Utils.h' for patching
1 out of 1 hunks FAILED -- saving rejects to file dom/bindings/Utils.h.rej
Keywords: checkin-needed
Created attachment 620712 [details] [diff] [review]
Exporting libc constants to JS

Propagated recent file and namespace renamings.
Attachment #619512 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5fd22efe23d

Should this have tests?
Flags: in-testsuite?
Keywords: checkin-needed
Target Milestone: --- → mozilla15
http://mozillamemes.tumblr.com/post/21637966463/yes-i-have-a-condition-that-forces-me-to-insert

Backed out due to bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/efd031e136ed

../../../dom/workers/WorkerScope.cpp: In function 'JSObject* mozilla::dom::workers::CreateDedicatedWorkerGlobalScope(JSContext*)':
../../../dom/workers/WorkerScope.cpp:992:42: error: 'DefineOSFileConstants' was not declared in this scope
Target Milestone: mozilla15 → ---
Created attachment 620976 [details] [diff] [review]
Exporting libc constants to JS

Headwall and apologies.
Attachment #620712 - Attachment is obsolete: true
Created attachment 622325 [details] [diff] [review]
Exporting libc constants to JS
Attachment #620976 - Attachment is obsolete: true
(In reply to Ryan VanderMeulen from comment #35)
> Should this have tests?

Probably not, this is just a set of constants definitions, split away from bug 707679.
Severity: normal → enhancement
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/685e5580a1f4
Flags: in-testsuite? → in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/685e5580a1f4
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
This doesn't build on VC9 because it doesn't have ELOOP, ELOOP, EOPNOTSUPP, EOVERFLOW, ETIMEDOUT or EWOULDBLOCK.
Ah, sorry, it was tested only on VC10. I will fix this.
Blocks: 754209
Depends on: 827193
You need to log in before you can comment on or make changes to this bug.