684017 - jsctypes: implement GetLastError and errno

Status: RESOLVED FIXED

(Core :: js-ctypes, enhancement)

Description

[Vadim Milichev]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/535.1 (KHTML, like Gecko) Chrome/13.0.782.215 Safari/535.1

Steps to reproduce:

I defined the winapi function using js-ctypes this way:

GetLastError: Libs.Kernel32.declare("GetLastError", WinABI, ctypes.uint32_t),

and call it after calls to another winapi functions (e.g. CreateFile).


Actual results:

GetLastError() always returns 0, despite if the previously called function succeeds.


Expected results:

GetLastError() should return error id, if the previously called function fails
http://msdn.microsoft.com/en-us/library/ms679360(v=vs.85).aspx

Comment 1

[Benjamin Smedberg]

As noted in the newsgroups, the correct way to fix this is to declare the prior function with a "needs errno" or "needs GetLastError" flag, as python does.

Comment 2

[Benjamin Smedberg]

See:

http://docs.python.org/library/ctypes.html#loading-shared-libraries
http://docs.python.org/library/ctypes.html#ctypes.get_errno
http://docs.python.org/library/ctypes.html#ctypes.get_last_error

Comment 3

[Bobby Holley (:bholley)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> As noted in the newsgroups, the correct way to fix this is to declare the
> prior function with a "needs errno" or "needs GetLastError" flag, as python
> does.

Seems reasonable - presumably there's a performance penalty to checking/storing errno and lastError unconditionally?

It looks like this option would have to go in Library.declare(). I'd prefer a more extensible approach to options than adding arguments willy-nilly to the type constructors.

The two approaches I can envision here are:
1 - Passing a single js 'options' object, where properties correspond to options.
2 - Adding mutators to the function types themselves, a la myFuncType.setNeedsErrno(bool).

I'd be tempted to use something like this in bug 599791 too (rather than adding yet another optional argument to the closure constructor), but I don't feel strongly about that.

Comment 4

[Benjamin Smedberg]

Yeah, if JS isn't going to grow named parameters any time soon we should just use an options object. There is a small performance penalty for GetLastError at least, although it would probably just be in the noise in the current impl using libffi. If we ever JIT ctypes calls, though, it may make a difference so I don't really want to bake it into the API.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Note: chances are that I will need this feature for the next incarnation of os.file, so I might start working on this soonish.

It looks like GetLastError and errno need some special treatment. What about exporting them both to a property |ctype.status|?

Comment 6

[Bobby Holley (:bholley)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #5)
> Note: chances are that I will need this feature for the next incarnation of
> os.file, so I might start working on this soonish.
> 
> It looks like GetLastError and errno need some special treatment. What about
> exporting them both to a property |ctype.status|?

That seems fine to me. Jorendorff?

Comment 7

[Jason Orendorff [:jorendorff]]

It depends. Why does GetLastError() always return 0? It seems to me it should work, and it might be better to figure out what's going on before plunging ahead with a solution.

errno and GetLastError() are two different things. Windows has both. It wouldn't do to expose them via a single property.

Exposing errno is not super useful without also exposing the errno constants or at least strerror(). Likewise GetLastError() and the Windows system error code constants.

Here are two possible designs:

  - provide a property ctypes.errno with a getter/setter that access the real
    errno for the current thread; and make sure GetLastError() works.

  - provide a property ctypes.errno and on Windows ctypes.lastError, initially 0,
    but not reflecting the real errno/LastError. Instead, after each C call,
    grab the values of errno and GetLastError(), store them in the properties,
    and set the real errno and LastError to 0.

It's a tossup to me. Instinctively I prefer the simpler and more direct approach; my only concern is that the JS engine might clear errno or the LastError unpredictably. It really shouldn't--that is a bad thing for code to do. Looking at what we've got, the only place we set errno = 0 is in js/src/shell/js.cpp, around I/O. That could end up inadvertently swallowing an error meant for ctypes, in the shell, but (a) who cares, and (b) it's easy enough to fix.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Jason Orendorff [:jorendorff] from comment #7)
> It depends. Why does GetLastError() always return 0? It seems to me it
> should work, and it might be better to figure out what's going on before
> plunging ahead with a solution.

> It's a tossup to me. Instinctively I prefer the simpler and more direct
> approach; my only concern is that the JS engine might clear errno or the
> LastError unpredictably. 

I think that this was more or less the premise of the bug when benjamin and I talked about it a few months ago. We do all sorts of stuff in ctypes, and I would imagine that something somewhere clears it.

> Here are two possible designs:
> 
>   - provide a property ctypes.errno with a getter/setter that access the real
>     errno for the current thread; and make sure GetLastError() works.
> 
>   - provide a property ctypes.errno and on Windows ctypes.lastError,
> initially 0,
>     but not reflecting the real errno/LastError. Instead, after each C call,
>     grab the values of errno and GetLastError(), store them in the
> properties,
>     and set the real errno and LastError to 0.

The second one is the only reason we'd need to support this explicitly in ctypes. If it shouldn't be needed (and is just indicative of a bug in our code), we should just figure that part out.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Stealing the bug.

Comment 10

[Jason Orendorff [:jorendorff]]

(In reply to Bobby Holley (:bholley) from comment #8)
> We do all sorts of stuff in ctypes, and
> I would imagine that something somewhere clears it.

...But what could that be? Nobody is supposed to clear errno unless they're actually catching an error. It's like writing try { ... } catch (exc) {} in JS. Fixing any places that do this would be the Right Thing.

I'm really surprised there are any; nothing in js/src touches errno except a few places in shell/js.cpp, around I/O, about what you'd expect.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Couldn't |jemalloc| clear |errno|?

Comment 12

[David Teller [:Yoric] - still alive but not very active]

(In reply to David Rajchenbach Teller [:Yoric] from comment #11)
> Couldn't |jemalloc| clear |errno|?

e.g. http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libc/stdlib/jemalloc.c?only_with_tag=MAIN

Comment 13

[Jason Orendorff [:jorendorff]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #12)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #11)
> > Couldn't |jemalloc| clear |errno|?
> 
> e.g.
> http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libc/stdlib/jemalloc.
> c?only_with_tag=MAIN

No. In fact, that very patch shows code where jemalloc goes out of its way to avoid clobbering errno!

If I remember correctly, the C standard explicitly states that no library function may have the effect of setting errno to zero.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Ah, my bad. Anyway, I have a prototype patch. I will upload it once it has been tested on Windows. Of course, if the GetLastError problem 1/ still appears; 2/ is due to js-ctypes itself, then it probably is a real bug, that should better be addressed by someone who knows their way around both js-ctypes and the ffi lib.

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Attachment: Implementation of ctypes.getStatus()

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 17

[David Teller [:Yoric] - still alive but not very active]

I am currently attempting to track down the original cause of GetLastError being reset to 0. So far, my best candidate is JSAutoSuspendRequest::~JSAutoSuspendRequest.

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Error traced to NSPR w95thread.c ( http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/md/windows/w95thred.c#304 ). This function calls Windows thread-local storage to determine the identity of the current thread (function TlsGetValue http://msdn.microsoft.com/en-us/library/windows/desktop/ms686812%28v=vs.85%29.aspx ), which resets the last error to 0.

So, for a quick fix, we can save the result GetLastError before the dstruction of JSAutoSuspendRequest and restore it afterwards. I will try this.

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Attachment: Prototype patch

As it turns out there are several nspr functions that can restore the Windows last error to 0 as a side-effect. I could hunt them all down and wrap them in a GetLastError/SetLastError block, but I have the impression that this is not the best solution. I attach here a patch that solves the issue as detailed above: by introducing a cross-platform function |ctypes.getStatus|, whose result is always the last |error|/|GetLastError| of a js-ctypes calls in the thread.

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Attachment: Prototype patch

Additional testing shows that the technique I was using to get the global |ctypes| object does not work if ctypes.jsm was imported in a limited scope, so here is a patch that uses a more robust (albeit a little surprising) technique to access that object. I now store the global |ctypes| in |CType.prototype|.

Comment 22

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 23

[Jason Orendorff [:jorendorff]]

Comment on attachment 602385 [details] [diff] [review]
Prototype patch

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

Always put the tests in the same patch. It’s easier on reviewers, but it also has other benefits.

::: js/src/ctypes/CTypes.cpp
@@ +59,5 @@
> +#if defined(XP_UNIX)
> +#include <errno.h>
> +#elif defined(XP_WIN)
> +#include <Windows.h>
> +#endif

The usual way to spell it is <windows.h>, lowercase.

@@ +97,5 @@
> +   * Get the global "ctypes" object.
> +   *
> +   * @param obj Any CType object. The function fails if |obj| is not a CType
> +   * object.
> +   * @return The global "ctypes" object. This is never NULL.

No javadoc please. Use English!

@@ +757,1 @@
>    // of [[Class]] "CTypeProto" that we fill in this automated manner.)

Oh - this comment is wrong. At least SLOT_CTYPES is not being filled with a CTypeProto object, and I think the INT64 slots aren't either. Please fix it!

@@ +3244,5 @@
> +  JS_ASSERT(CType::IsCType(obj));
> +
> +  JSObject *objTypeProto = JS_GetPrototype(obj);
> +  JS_ASSERT(objTypeProto);
> +  JS_ASSERT(CType::IsCTypeProto(objTypeProto));

Can you cause this assertion to fail by setting the __proto__ of a CType object?

@@ +5183,5 @@
> +#else // defined(XP_UNIX)
> +  const int status = 0;
> +  // Under non-Windows/Unix platforms, we have no mechanism for handling
> +  // ffi errors yet.
> +#endif // defined(XP_WIN)

Don’t all platforms have errno? It’s part of the C standard. Windows, certainly, has both.

It seems like if errno ever gets set to a nonzero value, it’ll be stuck that way. The patch doesn’t provide any way for the user to reset it to zero. So every future call will appear to fail. The same goes for the Windows GetLastError code, actually.

So I might write it like this:

    int savedErrno = errno, callErrno;
    errno = 0;
#ifdef XP_WIN
    int32_t savedLastError = GetLastError(), lastError;
    SetLastError(0);
#endif
    {
      JSAutoSuspendRequest ...
      ffi_call ...

      callErrno = errno;
      errno = savedErrno;
#ifdef XP_WIN
      // We need to save this before leaving the scope of |suspend| as
      // destructing |suspend| has the side-effect of clearing
      // |GetLastError| (see bug 684017).
      lastError = GetLastError();
#endif
    }
    SetLastError(savedLastError);
    ...

This way, ctypes.getStatus() always indicates the result of the most recent call. You can use it to ask “did that call succeed”? A nonzero status means it definitely failed; you're guaranteed the value isn’t left over from sloppy C/C++ code or a previous call. So no false positives, no false negatives, and no mess left over to confuse errno-using C/C++ code that isn’t as careful.

I think we should grab both error codes on Windows. They both exist, and they have different meanings and uses. It seems funny for something called ‘ctypes’ to expose errno everywhere except on Windows.

And it seems funny to expose GetLastError and errno with the same name. I think they don't really have enough semantics in common for it to be right to expose them under a single name--but really the question is, would your I/O code read better or worse, or neither, if the Windows code used ctypes.lastError and the Unix code used ctypes.errno? If neither, then this is all right.

Comment 24

[Jason Orendorff [:jorendorff]]

Comment on attachment 602386 [details] [diff] [review]
Companion testsuite

>+#include <Windows.h>

Lowercase <windows.h> here too.

We need a test that checks that errno doesn't get "stuck". (You can check that get_error_status returns 0 afterwards.)

r=me with that fixed.

Comment 25

[Jason Orendorff [:jorendorff]]

> No javadoc please. Use English!

BTW - the "mozilla coding style" page calls for javadoc comments, but SpiderMonkey has its own totally other coding style, for reasons that really aren't worth going into.

Comment 26

[David Teller [:Yoric] - still alive but not very active]

(In reply to Jason Orendorff [:jorendorff] from comment #23)
> Comment on attachment 602385 [details] [diff] [review]
> Prototype patch
> 
> Review of attachment 602385 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Always put the tests in the same patch. It’s easier on reviewers, but it
> also has other benefits.

Ok. I guess it is time for me to write my own team-rule-cheatsheet, to keep track of different conventions and habits between teams.

> No javadoc please. Use English!
Ok.
Are there any conventions on documentation in js/? I could not find any.

> @@ +757,1 @@
> >    // of [[Class]] "CTypeProto" that we fill in this automated manner.)
> 
> Oh - this comment is wrong. At least SLOT_CTYPES is not being filled with a
> CTypeProto object, and I think the INT64 slots aren't either. Please fix it!

I am not sure I understand your comment. If I read correctly, |CTypeProto| describes |proto| itself, not the content of |SLOT_CTYPES|.

> @@ +3244,5 @@
> > +  JS_ASSERT(CType::IsCType(obj));
> > +
> > +  JSObject *objTypeProto = JS_GetPrototype(obj);
> > +  JS_ASSERT(objTypeProto);
> > +  JS_ASSERT(CType::IsCTypeProto(objTypeProto));
> 
> Can you cause this assertion to fail by setting the __proto__ of a CType
> object?

It is my understanding that the __proto__ of a CType cannot be set, but if it was possible, I would say that yes, the assertion could fail. Do you think I should change the behavior?


> @@ +5183,5 @@
> > +#else // defined(XP_UNIX)
> > +  const int status = 0;
> > +  // Under non-Windows/Unix platforms, we have no mechanism for handling
> > +  // ffi errors yet.
> > +#endif // defined(XP_WIN)
> 
> Don’t all platforms have errno? It’s part of the C standard. Windows,
> certainly, has both.
> 
> It seems like if errno ever gets set to a nonzero value, it’ll be stuck that
> way. The patch doesn’t provide any way for the user to reset it to zero. So
> every future call will appear to fail. The same goes for the Windows
> GetLastError code, actually.

Two good points.

>     SetLastError(savedLastError);
>     ...

I doubt that |savedLastError| will be anything other than 0

> 
> This way, ctypes.getStatus() always indicates the result of the most recent
> call. You can use it to ask “did that call succeed”? A nonzero status means
> it definitely failed; you're guaranteed the value isn’t left over from
> sloppy C/C++ code or a previous call. So no false positives, no false
> negatives, and no mess left over to confuse errno-using C/C++ code that
> isn’t as careful.
> 
> I think we should grab both error codes on Windows. They both exist, and
> they have different meanings and uses. It seems funny for something called
> ‘ctypes’ to expose errno everywhere except on Windows.

Good point.

> And it seems funny to expose GetLastError and errno with the same name. I
> think they don't really have enough semantics in common for it to be right
> to expose them under a single name--but really the question is, would your
> I/O code read better or worse, or neither, if the Windows code used
> ctypes.lastError and the Unix code used ctypes.errno? If neither, then this
> is all right.

This would not really impact the code. Running through TryServer

My latest version (currently on TryServer) has |ctypes.errno| and |ctypes.lastError|. In the spirit of web-style prefixes and feature detection, perhaps we should rename that last one |ctypes.winLastError|. What do you think?

Comment 27

[David Teller [:Yoric] - still alive but not very active]

Attachment: Implementing ctypes.errno and ctypes.winLastError

Comment 28

[Jason Orendorff [:jorendorff]]

> > No javadoc please. Use English!
> Ok.
> Are there any conventions on documentation in js/? I could not find any.

No, not beyond what's in:
  https://wiki.mozilla.org/JavaScript:SpiderMonkey:C_Coding_Style

Don't worry about it. If you end up doing much more hacking in js/src, you'll pick it up. It's a fairly strong and consistent aesthetic. In short, we only comment stuff that's tricky-- which in CTypes.cpp is pretty much everything. We write comments for consumption by humans looking at code in a text editor.

> I am not sure I understand your comment. If I read correctly, |CTypeProto|
> describes |proto| itself, not the content of |SLOT_CTYPES|.

You're right.

> > > +  JS_ASSERT(CType::IsCTypeProto(objTypeProto));
> > 
> > Can you cause this assertion to fail by setting the __proto__ of a CType
> > object?
> 
> It is my understanding that the __proto__ of a CType cannot be set, but if
> it was possible, I would say that yes, the assertion could fail. Do you
> think I should change the behavior?

No, it's perfect. I forgot that CTypes are frozen.

> My latest version (currently on TryServer) has |ctypes.errno| and
> |ctypes.lastError|. In the spirit of web-style prefixes and feature
> detection, perhaps we should rename that last one |ctypes.winLastError|.
> What do you think?

I agree.

Comment 29

[David Teller [:Yoric] - still alive but not very active]

Attachment: Implementing ctypes.errno and ctypes.winLastError

Comment 30

[David Teller [:Yoric] - still alive but not very active]

Running one last check before checkin-needed.

Comment 31

[David Teller [:Yoric] - still alive but not very active]

Attachment: Implementing ctypes.errno and ctypes.winLastError

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85a83632b3c4

Comment 33

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/85a83632b3c4