1113379 - js/src/ctypes/CTypes.h: add NetBSD support

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[Thomas Klausner]

Attachment: Workaround for compilation problem described above.

The build of files including js/src/ctypes/CTypes.h fails on NetBSD due to:

>  0:12.55 /archive/foreign/firefox/js/src/ctypes/CTypes.cpp: In function ???bool js::ctypes::CanConvertTypedArrayItemTo(JSObject*, JSObject*, JSContext*)???:
>  0:12.55 /archive/foreign/firefox/js/src/ctypes/CTypes.cpp:2226:23: error: ???TYPE_int8_t??? was not declared in this scope
>  0:12.55      elementTypeCode = TYPE_int8_t;
>  0:12.56                        ^
>  0:12.56 /archive/foreign/firefox/js/src/ctypes/CTypes.cpp:2230:23: error: ???TYPE_uint8_t??? was not declared in this scope
>  0:12.56      elementTypeCode = TYPE_uint8_t;
>  0:12.56                        ^
>  0:12.56 /archive/foreign/firefox/js/src/ctypes/CTypes.cpp:2233:23: error: ???TYPE_int16_t??? was not declared in this scope
>  0:12.56      elementTypeCode = TYPE_int16_t;
>  0:12.56                        ^


and similar.
The reason is that NetBSD has a weird, but, I am told, standards-conformant way to define the basic integer types. From /usr/include/sys/types.h:

#ifndef int8_t
typedef __int8_t        int8_t;
#define int8_t          __int8_t
#endif

(and similar)

The attached patch works around this.

A discussion was started on a NetBSD mailing list to define them in another way, but I don't know the result of that yet, and it won't fix anything for NetBSD 6 and older releases:

http://mail-index.netbsd.org/tech-toolchain/2014/12/18/msg002479.html

Perhaps there's a better way to handle this in mozilla than the one I chose? Please let me know.

Comment 1

[Landry Breuil (:gaston)]

As i discussed with it privately with Thomas, i think at some point i had a similar failure, needed more or less the same patch, and it the end it wasnt needed. Maybe that was at the moment where we had a mix between jsuint, pruint and uint_t.... who should we cc specifically for js/ctypes bugs ?

Comment 2

[Landry Breuil (:gaston)]

Sidenote - the attached patch also contains the one from 1113374

Comment 3

[Thomas Klausner]

Attachment: Minimized diff

Sorry, as Landry reported I had two patches in here.
I have reduced it to only the one that's relevant for this bug report.

Comment 4

[Landry Breuil (:gaston)]

Comment on attachment 8539727 [details] [diff] [review]
Minimized diff

I'm honestly not sure it's a proper fix... but lets ask for feedback/review :)

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8539727 [details] [diff] [review]
Minimized diff

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

Oh, wow. Why, NetBSD, why?

Deflecting review to Jeff, who may know a better C preprocessor trick.

Please put the hack in ctypes/typedefs.h, instead of CTypes.h, as typedefs.h contains the troublesome uses of uint8_t and friends as preprocessor tokens.

Comment 6

[Thomas Klausner]

Thanks for the review, :jorendorff.

When I try to move the code out of CTypes.h, I get:

 3:01.03 In file included from /archive/foreign/firefox/js/src/ctypes/Library.cpp:11:0:
 3:01.03 /archive/foreign/firefox/js/src/ctypes/CTypes.h:232:1: error: expected initializer before ‘}’ token
 3:01.03  };
 3:01.03  ^
 3:01.03 /archive/foreign/firefox/js/src/ctypes/CTypes.h:304:3: error: ‘Array’ does not name a type
 3:01.03    Array<JS::Heap<JSObject*> > mArgTypes;
 3:01.03    ^
 3:01.03 /archive/foreign/firefox/js/src/ctypes/CTypes.h:309:3: error: ‘Array’ does not name a type
 3:01.03    Array<ffi_type*> mFFITypes;
 3:01.03    ^
 3:01.03 /archive/foreign/firefox/js/src/ctypes/CTypes.h:446:5: error: ‘TypeCode’ has not been declared
 3:01.03      TypeCode type, JSString* name, jsval size, jsval align, ffi_type* ffiType);
 3:01.03      ^
 3:01.04 /archive/foreign/firefox/js/src/ctypes/CTypes.h:449:65: error: ‘TypeCode’ has not been declared
 3:01.04      JSObject* typeProto, JSObject* dataProto, const char* name, TypeCode type,
 3:01.04                                                                  ^
 3:01.04 /archive/foreign/firefox/js/src/ctypes/CTypes.h:454:3: error: ‘TypeCode’ does not name a type
 3:01.04    TypeCode GetTypeCode(JSObject* typeObj);
 3:01.04    ^
 3:01.04 /archive/foreign/firefox/js/src/ctypes/CTypes.h:501:5: error: ‘AutoCString’ has not been declared
 3:01.04      AutoCString& result);
 3:01.04      ^
 3:01.04 /archive/foreign/firefox/js/src/ctypes/CTypes.h:534:1: error: expected declaration before ‘}’ token
 3:01.04  }
 3:01.04  ^

So I think it has to stay there. Am I overlooking something?

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Better approach, seems to build a CTypes.cpp with NetBSD-style craziness added to it, pending tryserver

Direct concatenation of a macro argument prevents it from being expanded.  So if a macro is invoked with int8_t as an argument, and that macro *directly* (not by delegation to another macro) concatenates the relevant argument with some other token, no expansion occurs.  So if we could get DEFINE_INT_TYPE invoked *directly* with int8_t as argument for these TYPE_##type buildup cases, we'd be good.

We can do this with the higher-order macro trick first started a bit ago by jorendorff, now rapidly propagating through the JS engine.  I hadn't realized higher-order macros were more powerful than macro tables in this regard.  Useful to know!  (Not that I won't cast aspersions at NetBSD's crazy <stdint.h> just like all of you have.  ;-) )

My means of testing this locally was to build with this added to the top of CTypes.cpp:

    #include <stdint.h>

    typedef int8_t  SuperCrazyInt8Type;

    #define int8_t SuperCrazyInt8Type

even before the #include "ctypes/CTypes.h".  A build with that failed.  A build with this patch applied atop that builds CTypes.cpp.  I'm still churning on a full browser build, and if that succeeds it's off to try for a full check everywhere.

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 8539727 [details] [diff] [review]
Minimized diff

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

::: js/src/ctypes/CTypes.h
@@ +16,5 @@
>  
> +#if defined(__NetBSD__)
> +#include <stdint.h>
> +/* XXX why do we have those funky __ #defines in stdint.h? */
> +#warning this is a retarded workaround

I don't think we're taking this patch, given a better alternative's apparent success in a local build.  But for future reference.

First.  If such a hack were to be added, the comment by it wouldn't be to ask why we were doing this -- it would be to explain why.  A link to the NetBSD message, and perhaps mentioning that int8_t is a #define of something weird on NetBSD, such that CTypes.cpp's attempts to concatenate such a token with TYPE_ causes the result to not be TYPE_int8_t, but TYPE___int8_t (TYPE_ # __int8_t), would be better.

Second.  I'm all for artfully casting side-eyed aspersions at weird code in comments.  ;-)  But it should be a comment, not a #warning that every single person who builds ctypes will see as warning/log-pollution in their build.  Buried treasure for the curious reader to find and be suitably entertained/horrified to see.  :-)  I guarantee that if this landed, there'd be a bug in days about the new warning in build output, and we'd then be changing this to not be a #warning.

Third.  There are cleverer ways to cast such aspersions than to describe code working around them as "retarded".  There's no reason to assume you're saying anything about, let alone denigrating, anyone's mental acuity (instead of simply using a metaphor).  Nonetheless, there's a certain set of people who will jump to that conclusion, or be offended nonetheless at seeing this.  And there are plenty of other ways to be equally forceful.  So best not to describe it that way.

Comment 9

[Thomas Klausner]

Thanks, :Waldo, for both the new patch and the comments.

I took the patch from pkgsrc as-is, but I should have looked at the language. I'll be more careful in the future, I agree that this is not wording I would want to find in source code (comments).

As for your suggested patch: I can confirm that this fixes the compilation for me on NetBSD-7.99.3/amd64.
I'd be happy to see this committed!

Comment 10

[Landry Breuil (:gaston)]

Trying a build on OpenBSD/amd64 with that patch, and also sent to try:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=19730438830f

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Also update ctypes test code

Feh, didn't notice typedefs.h includes far afield from js/src/ctypes.  The test changes there, in a couple places, run into the problem of int8_t being passed down into a delegating macro, as originally reported here.  The hackaround here -- concatenate that name with _, then pass in int8_t_ to an implementation macro -- isn't pretty.  It still seems nicer than what was done here (which, incidentally, I guess would have to be done to jsctypes-test.* as well for a full fix -- I guess they never build with tests?).

Comment 12

[Landry Breuil (:gaston)]

Yeah, tpbl was burning with the first patch, see https://tbpl.mozilla.org/php/getParsedLog.php?id=56080644&tree=Try.. but my clobber build on OpenBSD succeeded :)
Should i do a try run with clobber ?

Comment 13

[Jeff Walden [:Waldo]]

Shouldn't be necessary.  I did a try-push of almost the patch posted just now, missing only the comment explaining the nameAndUnderscore hack (added as an afterthought), that gave an all-clear, so I think we're set.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=314aad31b08e

Comment 14

[Landry Breuil (:gaston)]

 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2a3ee6726442 has att 8546523

Comment 15

[Landry Breuil (:gaston)]

ooops... should have waited a bit more to preserve resources... cancelling my try run then.

Comment 16

[Jason Orendorff [:jorendorff]]

Comment on attachment 8546523 [details] [diff] [review]
Also update ctypes test code

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

I don't understand what preprocessor black magic causes the macro `int8_t` to be expanded in some cases but not others, but I don't want to hold this up any longer.

Comment 17

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be02b84dfca4

The preprocessor magic is fairly simple.  From this, first sentence:

https://gcc.gnu.org/onlinedocs/cpp/Argument-Prescan.html#Argument-Prescan

"Macro arguments are completely macro-expanded before they are substituted into a macro body, unless they are stringified or pasted with other tokens."  So if I have

#define MACRO 42
#define FOO(x) x

then FOO(MACRO) will expand to 42.  In FOO(MACRO), MACRO is provided as an argument that's a macro.  The corresponding argument in FOO, x, is not stringified or pasted onto another token in FOO.  Therefore MACRO is completely macro-expanded, then substituted into FOO's body: we substitute 42 for x, to ultimately get 42.

But if instead I do

#define MACRO 42
#define FOO(x) x ## _suffix

then FOO(MACRO) will be MACRO_suffix.  In FOO(MACRO), MACRO is still an argument that's a macro.  But the corresponding argument x in FOO *is* pasted onto another token in FOO.  So MACRO is not macro-expanded, just substituted directly for x: MACRO ## _suffix becomes MACRO_suffix.

In the original code, all the various types were written out in typedefs.h as

DEFINE_INT_TYPE(int8_t, int8_t, ffi_type_sint8)

and so on.  DEFINE_INT_TYPE was in turn defined as

#define DEFINE_INT_TYPE(x, y, z) DEFINE_TYPE(x, y, z)

Because the x/y/z in DEFINE_INT_TYPE's body were *not* stringified or pasted in that body, they're expanded to however they were defined, resulting in int8_t being expanded to produce

DEFINE_TYPE(__int8_t, __int8_t, ffi_type_sint8)

as representative of the output of the various type macro calls.  Thus the problem.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/be02b84dfca4

Comment 19

[Jason Orendorff [:jorendorff]]

Thanks for the edifying explication!