Closed Bug 556329 Opened 14 years ago Closed 14 years ago

Allow ctypes to load exported data symbols

Categories

(Core :: js-ctypes, defect, P2)

Product:
Core
Component:
js-ctypes
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: dwitte, Assigned: dwitte)

Details

Attachments

(1 file, 1 obsolete file)

patch v1.1
10.89 KB, patch
dwitte
: review+
Details | Diff | Splinter Review 
Way back when, we opined that the ability to import data symbols would be nice, and now, sheppy has hit this for real.

From a CF header:

  CF_EXPORT
  const CFArrayCallBacks kCFTypeArrayCallBacks;

Good luck using that in ctypes; if you can't hack around it, you're hosed.

We *might* be able to use PR_FindSymbol to pull in static data, but something tells me that's thoroughly optimistic. If not, we gotta go add our own NSPR bits for this. Or just do it ourselves.

As for syntax, we could overload library.declare:

  let CFArrayCallBacks = ctypes.StructType(...);
  let kCFTypeArrayCallBacks = library.declare("kCFTypeArrayCallBacks", CFArrayCallBacks);

P2, nice for 1.9.3; this could be P1, though... I'm not sure how common (and how much of a hard requirement, for some applications) this is.
PR_FindSymbol should work, it should give you the address of the static data symbol.
Have patch, testing now.
Attached patch patch v1 (obsolete) — — Splinter Review 
This should do it. Syntax in comment 0.

This applies to mozilla-central as of thismorning, but you can apply it to an older pull just fine (just ignore the apply failure in test_jsctypes.js.in).
Assignee: nobody → dwitte
Status: NEW → ASSIGNED
I'm doing a build with this patch on it and will give it a try with my demo to see how it goes.
Getting this odd error trying to build with this patch applied:

configuring in js/ctypes/libffi
running /bin/sh /Volumes/HardWorker/mozilla/mozilla-central/js/ctypes/libffi/configure  --disable-shared --enable-static --disable-raw-api --enable-debug --with-pic --build=x86_64-apple-darwin10.3.0 --host=i386-apple-darwin8.0.0 HOST_CC="gcc-4.2" CC="gcc-4.2 -arch i386" --cache-file=../../../js/ctypes/libffi/config.cache --srcdir=/Volumes/HardWorker/mozilla/mozilla-central/js/ctypes/libffi
configure: loading cache ../../../js/ctypes/libffi/config.cache
configure: error: `build_alias' has changed since the previous run:
configure:   former value:  `x86_64-apple-darwin10.2.0'
configure:   current value: `x86_64-apple-darwin10.3.0'
configure: error: in `/Volumes/HardWorker/mozilla/mozilla-central/obj-ff-dbg/js/ctypes/libffi':
configure: error: changes in the environment can compromise the build
configure: error: run `make distclean' and/or `rm ../../../js/ctypes/libffi/config.cache' and start over
configure: error: /Volumes/HardWorker/mozilla/mozilla-central/js/ctypes/libffi/configure failed for js/ctypes/libffi
*** Fix above errors and then restart with               "make -f client.mk build"
make[1]: *** [configure] Error 1
make: *** [/Volumes/HardWorker/mozilla/mozilla-central/obj-ff-dbg/Makefile] Error 2

There is no config.cache file. Not sure what's going on with this. Any thoughts?
Blow away your objdir and build again?
Was hoping to avoid a full rebuild, but doing that now.
I nuked the objdir, and started getting this error instead next time I tried a build:

configure: error: source directory already configured; run "make distclean" there first
configure: error: /Volumes/HardWorker/mozilla/mozilla-central/js/ctypes/libffi/configure failed for js/ctypes/libffi
*** Fix above errors and then restart with               "make -f client.mk build"
make[1]: *** [configure] Error 1
make: *** [/Volumes/HardWorker/mozilla/mozilla-central/obj-ff-dbg/Makefile] Error 2

I get the same error if I do a make -f client.mk distclean too.
Uh... did you build in the srcdir at some point? If so, maybe 'cd srcdir/js/ctypes/libffi && make distclean' will help, I'm not sure. If that doesn't, probably easiest just to re-pull.

You could also try 'hg status js/ctypes/libffi', and delete all the files it has '?' for.
OK, got it built with the patch. I get an error saying that my lib.declare for the callback structure needs to have at least three parameters, but the syntax in comment #0 shows just two.
Hmm, that must mean you haven't built with the patch, then -- there's no string left in the code that says you need at least three parameters. :/

Can you double check that, and paste your js?
Hm. I did:

MacXTreme:mozilla-central sheppy$ patch -p1 <~/Downloads/attachment.cgi 
patching file js/ctypes/CTypes.cpp
patching file js/ctypes/Library.cpp
patching file js/ctypes/tests/jsctypes-test.cpp
patching file js/ctypes/tests/jsctypes-test.h
patching file js/ctypes/tests/unit/test_jsctypes.js.in

Wouldn't that mean I've patched it?

If I try the patch command again, I get told that it's already been applied. So I've definitely applied the patch. Might you have missed a file when generating the patch?
Your patch output looks good, it's definitely applied right. How did you rebuild, exactly?
From start to finish -

1. Backed up my mozconfig file.
2. Deleted my entire mozilla-central tree.
3. hg clone http://hg.mozilla.org/mozilla-central/ mozilla-central
4. Copied my mozconfig back into place.
5. Applied the patch.
6. make -f client.mk build

That's it.
Bah, I just realized I'm running the old copy of Firefox I built previously. Testing for real now.
Woot - iPhoto launched. I need to change my code to download the image and pass that in instead of the URL to the image on the web though, but at least the callback structure works now. So this seems to be working for me.
Comment on attachment 436557 [details] [diff] [review]
patch v1

Awesome. :)

I'd like jorendorff to take a look at this. I'm torn on how to make static data accessible:

1) Roll a CData that refers to it (i.e. doesn't own), and seal it. People could still write to the static data by doing .address().contents. (This is what the patch currently does.)

2) Copy the data. Safer, but also means people... can't modify static data. There may be cases where that's useful. It also would be less efficient for large static data sets, but I doubt that's common.

Doing 2) has the added advantage that we don't need to special-case the 'owns' bit based on whether we're looking at a function pointer or other data, which kinda sucks.

I think we should just do 2) unless there's a compelling reason to allow writing.
Attachment #436557 - Flags: review?(jorendorff)
Can't we just expose it as a pointer?
So the result of library.declare("...", t) is of type t.ptr? We could, but that'd still allow writing... it just removes the cost of copying.

I should also add to 1) that it would allow the library itself to modify data, and have it reflected in the CData object. Doing 2) prevents that, unless one re-declares it.
We need to allow writing, I think... setting exported global variables isn't totally uncommon.
Agreed; need to be able to write.
OK, t.ptr would work nicely then.

(Then could solve the FunctionType vs. other data inconsistency by making FunctionType a pointer... mumble mumble!)
Just to add, my "add image to iPhoto" sample extension is now working correctly, but I'll hold off on starting to write the article around it until this is finalized.
Comment on attachment 436557 [details] [diff] [review]
patch v1

Just two nits:

> JS_ASSERT(!refObj || !CData::IsCData(cx, refObj) || !ownResult);

JS_ASSERT_IF(refObj && CData::IsCData(cx, refObj), !ownResult);

> *root.addr() = OBJECT_TO_JSVAL(typeObj);

root.setObject(typeObj);

r=me with those changes.
Attachment #436557 - Flags: review?(jorendorff) → review+
Do we have a guess as to when this will land?
Attached patch patch v1.1 — — Splinter Review 
A few more tests and some tweaks over previous patch. Also relaxes the immutability condition on the resulting CData object...
Attachment #436557 - Attachment is obsolete: true
Attachment #438615 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Hm, this has been checked in, apparently, but doesn't seem to work in nightlies of Minefield.

    Application.console.log("Creating callback struct");
    this.CFArrayCallBacks = new ctypes.StructType("CFArrayCallBacks",
      [ {'version': CFIndex},
        {'retain': ctypes.voidptr_t},
        {'release': ctypes.voidptr_t},
        {'copyDescription': ctypes.voidptr_t},
        {'equal': ctypes.voidptr_t} ]);

    Application.console.log("Declaring pointer to struct");
    this.kCFTypeArrayCallBacks = this.lib.declare("kCFTypeArrayCallBacks",
                              this.CFArrayCallBacks);
    Application.console.log("Done");

In this code snippet, I get the "Creating callback struct" and "Declaring pointer to struct" outputs, but not "Done". So either I have a syntax error in the declare line that I can't find, or this isn't on trunk yet. Any idea which it is?
OK, I see that this has been merged into m-c now, judging from the build I just did, on which my sample works.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: