415289 - Port Dehydra GCC to Mac OS X

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Stan Shebs]

The GCC plugin patch will need some work to be adapted to Apple's version of GCC, which is based on an older version.

Comment 1

[Benjamin Smedberg]

We should probably target the (newer) apple gcc 4.2 prerelease which gets visibility correct, not the gcc 4.0 release version which is unhappily old.

Comment 2

[Stan Shebs]

Probably so, still digging through versions (and discovering that my entry in gcc/MAINTAINERS was overzealously deleted, ha ha).

Comment 3

[Vlad Sukhoy]

Attachment: vlad's patch for plugin.diff v1

This is the patch for plugin.diff of dehydra-gcc that makes it applicable on gcc42 preview 1 sources from apple. After patched plugin.diff is applied on that source, it is possible to build gcc and run it with dehydra as a plugin (another patch coming shortly).

One notable change that I had to make is replacing RTLD_LAZY with RTLD_NOW in dlopen() call, for some reason delayed loading of dehydra's functions which I built in a bundle did not work with RTLD_LAZY. No idea about why this happens.

Comment 4

[Vlad Sukhoy]

for the record: another issue with the patch is that it makes all column number in dehydra's location output be zero.

Comment 5

[Vlad Sukhoy]

Attachment: vlad's patch for dehydra v1 (wip)

The patch that makes it build as a bundle on macos and work with cc1plus (not with g++, however). SpiderMonkey from darwin ports (sudo port install spidermonkey) seems to be ok, if it is installed then dehydra may be configured via

./configure --js-libs=/opt/local/lib/ --js-headers=/opt/local/include/js/

and invoked as usual (except that it will trigger assert in dehydra on almost any source code with a few exceptions). Here's what gmake -k -C test does for me:

../../gcc-build/gcc/cc1plus -quiet -fplugin=../gcc_dehydra.so -fplugin-arg=test.js test.cc -o /dev/null
dehydra_ast.c:31: Assertion failed:length < dehydra_getArrayLength (this, this->destArray)
gmake: *** [test.o] Error 1
gmake: Leaving directory `/Users/sukhoy/Mozilla/gcc-dehydra/dehydra-gcc/test'
sweedom:dehydra-gcc sukhoy$ gmake -k -C test
gmake: Entering directory `/Users/sukhoy/Mozilla/gcc-dehydra/dehydra-gcc/test'
../../gcc-build/gcc/cc1plus -quiet -fplugin=../gcc_dehydra.so -fplugin-arg=test.js test.cc -o /dev/null
dehydra_ast.c:31: Assertion failed:length < dehydra_getArrayLength (this, this->destArray)
gmake: *** [test.o] Error 1
../../gcc-build/gcc/cc1plus -quiet -fplugin=../gcc_dehydra.so -fplugin-arg=test.js assign.cc -o /dev/null
dehydra_ast.c:31: Assertion failed:length < dehydra_getArrayLength (this, this->destArray)
gmake: *** [assign.o] Error 1
../../gcc-build/gcc/cc1plus -quiet -fplugin=../gcc_dehydra.so -fplugin-arg=test.js typedef.cc -o /dev/null
../../gcc-build/gcc/cc1plus -quiet -fplugin=../gcc_dehydra.so -fplugin-arg=test.js operator_new.cc -o /dev/null
../../gcc-build/gcc/cc1plus -quiet -fplugin=../gcc_dehydra.so -fplugin-arg=test.js destr_order.cc -o /dev/null
../../gcc-build/gcc/cc1plus -quiet -fplugin=../gcc_dehydra.so -fplugin-arg=test.js init.cc -o /dev/null
../../gcc-build/gcc/cc1plus -quiet -fplugin=../gcc_dehydra.so -fplugin-arg=test.js constructor.cc -o /dev/null
dehydra_ast.c:31: Assertion failed:length < dehydra_getArrayLength (this, this->destArray)
gmake: *** [constructor.o] Error 1
../../gcc-build/gcc/cc1plus -quiet -fplugin=../gcc_dehydra.so -fplugin-arg=test.js types.cc -o /dev/null
../../gcc-build/gcc/cc1plus -quiet -fplugin=../gcc_dehydra.so -fplugin-arg=test.js templ-spec.cc -o /dev/null
../../gcc-build/gcc/cc1plus -quiet -fplugin=../gcc_dehydra.so -fplugin-arg=test.js stack_fieldOf.cc -o /dev/null
../../gcc-build/gcc/cc1plus -quiet -fplugin=../gcc_dehydra.so -fplugin-arg=test.js templ-simple.cc -o /dev/null
../../gcc-build/gcc/cc1plus -quiet -fplugin=../gcc_dehydra.so -fplugin-arg=test.js longevity.cc -o /dev/null
dehydra_ast.c:31: Assertion failed:length < dehydra_getArrayLength (this, this->destArray)
gmake: *** [longevity.o] Error 1
../../gcc-build/gcc/cc1plus -quiet -fplugin=../gcc_dehydra.so -fplugin-arg=test.js finalizers.cc -o /dev/null
dehydra_ast.c:31: Assertion failed:length < dehydra_getArrayLength (this, this->destArray)

(by uncommenting print's in test/test.js one can verify that something is indeed going on :))

Unfortunately, gcc42 preview 1 is quite different from the gcc that dehydra was using, so I had to modify dehydra's source several times. There were some weird header conflicts between SM and gcc 42 preview which had to be handled. More importantly, the tree handling code had to be changed too, as several macro's and functions that dehydra was using are not available in this gcc. The assertion above seems to be caused by inadequate conversion of the code in dehydra_ast.c which handles CALL_EXPR, but I suspect there are other glitches too.

Comment 6

[Vlad Sukhoy]

Attachment: vlad's patch for dehydra v2

Fixed an error in processing of CALL_EXPR which caused those assertions, tests now run and produce output mostly similar to what dehydra on linux does (when print's in test.js are uncommented). Column numbers are no longer printed (no such info provided by gcc42 preview in location data structure). Now I'll see if it is possible to do http://developer.mozilla.org/en/docs/Building_with_static_checking on a mac with this dehydra.

Comment 7

[Vlad Sukhoy]

Correction: the patched MacOS dehydra does run with g++, not only with cc1plus, I did not invoke "gmake install" when rebuilding gcc last time and it somehow caused that error.

Comment 8

[David Mandelin [:dmandelin]]

Good work. I'm surprised there are so many API differences.

I also wanted to mention that you may be able to work around the GTY stuff temporarily. It's only used to build treehydra: dehydra is used to analyze the GTY structures and print out C functions to convert GTY structs to JS in treehydra_generated.c. So you may be able to (a) run this step on Linux against your Mac sources, or (b) run it on Mac with GTY patch applied, then unapply the GTY patch in order to build GCC. Method (b) was actually used in early prototypes of Linux treehydra.

Comment 9

[Vlad Sukhoy]

(In reply to comment #8)
> Good work. I'm surprised there are so many API differences.

Significant part of them are trivial (function or macro got renamed, commonly used expression became a macro of its own in gcc etc.) and can be handled within some sort of minimalistic "uniform gcc tree layer" if desired. Some aren't (e.g. "no node of this kind in gcc 42"), I suppose these particular cases have to be looked into in more detail.

> So you may be able to (a) run this step on Linux against your Mac sources, or (b) run it on Mac with GTY patch applied

I'll see what happens if we try (b) and if that doesn't work, revert to (a).

Comment 10

[Stan Shebs]

Vlad, I'm guessing you did a single stage 1 build of the the patched compiler? When I do a "make install", which is how Apple builds installable packages from source, it does a bootstrap, which compiles with -Werror, which ultimately results in a complaint about mismatched types in tree-plugin-pass.c . The fix is to move the <dlfcn.h> to the top of the source file, the problem being that "bool" has one declaration in Apple headers and a different one in GCC headers. So we want to keep Apple's declaration from getting in between prototypes and function definitions in the plugin sources.

Comment 11

[Vlad Sukhoy]

(In reply to comment #10)
> Vlad, I'm guessing you did a single stage 1 build of the the patched compiler?
Yes, that's what I did. I guess we have to make sure that the patched compiler bootstraps correctly..

Comment 12

[Vlad Sukhoy]

Failed to reproduce this error with mismatched types in tree-plugin-pass.c Perhaps we are using different versions of dlfcn.h or of something included by it. Anyhow, I'll update the patch with that change (after verifying that this updated version bootstraps in my environment)..

Comment 13

[Vlad Sukhoy]

Attachment: patch for plugin.diff v2

Comment 14

[Vlad Sukhoy]

While experimenting with static checking build of mozilla on mac I hit this:

In file mmintrin.h the following snippet of code causes assertion in dehydra_makeVar and therefore breaks the build:

typedef long long __m64 __attribute__ ((__vector_size__ (8), __may_alias__));

static __inline __m64
_mm_setzero_si64 (void)
{
  return (__m64)0LL;
}

copied this code to linux - still the same assertion.

Comment 15

[(dormant account)]

Hey Vlad,
thanks for the testcase. Fixed.

As for the rest, excellent work. I'll start reviewing this soon. For now my main recommendation is to define missing macros instead of changing them

For example:
#define GENERIC_TREE_OPERAND TREE_OPERAND..though in this case you might need to actually implement the generic_tree_operand function.

Comment 16

[Vlad Sukhoy]

> Fixed.
Sweet.

> For now my main recommendation is to define missing macros instead of changing them

OK.

Comment 17

[Vlad Sukhoy]

Attachment: patch for dehydra v3 (wip)

Static checking build of browser from mozilla-central was successful with this dehydra (I was able to run Minefield). A couple of bugs in the tree handling code had to be fixed, one particular issue was accessing freed memory in gcc_plugin_finish_struct after gcc_plugin_post_parse was called (not sure if this can happen on linux, but if it can then tree_queue_tail points to freed memory). If this cannot happen on linux then probably we should update plugin.diff to make sure it cannot happen on mac too..
The code in the patch is wip.


For MacOS build to work, I had to do the following:
 * build patched gcc with objective c++ support.
 * copy libstdc++.dylib from gcc42 preview 1 binary installation to where patched gcc is installed (for me it was .../installed/lib/gcc/i386-apple-darwin9.2.2/4.2.1 ), as building libstdc++.dylib from source was problematic for me for some reason.
 * the following is the relevant part of mozconfig that had to use due to various header issues:

GCC_PREFIX=$HOME/Mozilla/gcc-dehydra/installed
CXX=$GCC_PREFIX/bin/g++
export CXXFLAGS="-I $GCC_PREFIX/lib/gcc/i386-apple-darwin9.2.2/4.2.1/include \
-I /usr/include/c++/4.0.0/i686-apple-darwin9 \
-I /usr/include/c++/4.0.0/backward/ \
-I /usr/include/c++/4.0.0 \
-Wno-deprecated"
ac_add_options --with-static-checking=$HOME/Mozilla/gcc-dehydra/dehydra-gcc/gcc_dehydra.so

Finally, I had to make the following change in gfxPlatformMac.h:

-    already_AddRefed<gfxASurface> gfxPlatformMac::OptimizeImage(gfxImageSurface *aSurface,
-                                                                gfxASurface::gfxImageFormat format);
+    already_AddRefed<gfxASurface> OptimizeImage(gfxImageSurface *aSurface,
+                                                gfxASurface::gfxImageFormat format);

and

-    void gfxPlatformMac::AppendCJKPrefLangs(eFontPrefLang aPrefLangs[], PRUint32 &aLen, 
+    void AppendCJKPrefLangs(eFontPrefLang aPrefLangs[], PRUint32 &aLen, 


I checked if at least something in static-checking.js actually works - indeed, creating an instance of nsQueryInterface on the heap triggered appropriate error in js, which is encouraging.

Comment 18

[Stan Shebs]

For my part, my Apple-style build installs as "g++-4.2" with everything in the right place, so no fooling with -I flags, but I did have to edit build_gcc so as not to strip executables. But in building mozilla, I get an assertion fail in dehydra_makeVar while compiling nsSVGFilters.cpp.

Comment 19

[Stan Shebs]

Attachment: Stack trace from the xassert

Here is the gdb session with stack trace for the assert failure.

Comment 20

[Vlad Sukhoy]

Attachment: patch for dehydra v4

This patch is hopefully close to being it modulo review times whatever pops up. The following was added:
 * so_suffix and shared link flags setup in configure script.
 * missing macro's are now defined where feasible and some code was added to handle significant differences in the tree structure between gcc's we are working with.
 * added some code to handle mapped (linux) and non-mapped (mac) locations.
 * AGGR_INIT_EXPR and CALL_EXPR mac handling was significantly altered compared to previous patch (this part was sort of broken in previous patches).

With this patch dehydra should
 * build and run on mac; build mozilla-central with static checking.
 * build and run on linux.

I also attempted to compare what happens in JS between two platforms using the following method: uncomment print's in test.js, for each test file in dehydra's test dir run the script to: redirect js output into a file, replace "," with ",\n" and compare results from linux and mac using diff. No significant differences were found except the obvious fact that mac dehydra does not print column numbers (testing of this sort may run fully automated which may be one way to protect against dehydra platform regressions as the tool matures).

The patch depends on the fix in dehydra that was pushed today.

Comment 21

[Vlad Sukhoy]

Attachment: configure patch (split from dehydra v4)

Comment 22

[Vlad Sukhoy]

Attachment: source patch v5 (split from dehydra patch v4)

Split patch as requested in IRC.

Comment 23

[(dormant account)]

Comment on attachment 312787 [details] [diff] [review]
source patch v5 (split from dehydra patch v4)

>diff -r 8e457aa1c58e cp-tree-workaround.h
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/cp-tree-workaround.h	Fri Mar 28 21:34:12 2008 -0500
>@@ -0,0 +1,7 @@
>+/* A workaround for weird conflict between Apple GCC 4.2 Preview 1's cp-tree.h
>+   and SpiderMonkey's jsotypes.h */
>+#define uint32 unsigned long
>+#define uint16 unsigned short
>+#include <cp-tree.h>
>+#undef uint32
>+#undef uint16

This seems wrong. 
a) Should include jsapi.h in the same file..perhaps call it cp-tree-jsapi-workaround.h :)
b) uint32 wont be an unsigned long on amd64. What exactly is the error on OSX, is it possible to do less hardcoding of types here?

Comment 24

[(dormant account)]

Comment on attachment 312433 [details] [diff] [review]
patch for dehydra v4

>+#ifdef AGGR_INIT_EXPR_PROCESS_INPLACE
>+      // the arguments are not operands but a chain, like in apple gcc42
>+      dehydra_attachNestedFields(this, obj, ARGUMENTS,
>+				 aggr_init_expr_args(*tp));
>+#else
>+      // the arguments are operands
>+      dehydra_fcallDoArgs (this, obj, *tp, aggr_init_expr_first_param_index,
>+			   TREE_OPERAND_LENGTH(*tp));
>+#endif

I'm impressed at the hoops you managed to jump through to do call expressions. However, I think it'd be clearer if we namespaced the conditional macros, so it's more obvious which ones are from dehydra and which are from GCC.

AGGR_INIT_EXPR_PROCESS_INPLACE + CALL_EXPR_PROCESS_INPLACE -> DEHYDRA_INPLACE_ARGS ?

Comment 25

[(dormant account)]

Other than these minor issues good. Please add a check for DARWIN before #defining DEHYDRA_INPLACE_ARGS and error() if that's not the case.

Comment 26

[Stan Shebs]

Attachment: patch for apple gcc 4.2-5531

Here is my patch for gcc_4.2-5531, aka "4.2 preview 1". Just a couple changes from Vlad's patch, but known to build an installable compiler. Basically one applies this to a source tree for gcc_4.2-5531, then "sudo make install" and "sudo ditto dst/ /", which installs the compiler in /usr/bin as gcc-4.2. Most importantly, all the headers and libraries are installed in their correct places, so you just need to say "gcc-4.2" or "g++-4.2" and it all works.

Comment 27

[Vlad Sukhoy]

Attachment: source patch v6

The problem with conflicting headers is that unless uint32 is defined to something, apple gcc headers typedef uint32 and uint16. This patch does workaround in a better way, as requested.

Added other stuff as requested (__APPLE_CC__ is used to instead of generic darwin check as suggested in IRC).

Comment 28

[(dormant account)]

This last patch seems good to me. Dave, can you check it out too?

Comment 29

[(dormant account)]

Attachment: make dehydra compile with vlad's patch

gcc_cp_headers.diff is special and therefore fragile :). This fixes treehydra build when vlad's patch is applied

Comment 30

[(dormant account)]

Applied

Comment 31

[Vlad Sukhoy]

We'll port treehydra here too.

Comment 32

[Vlad Sukhoy]

For the record: the mac port of dehydra does not handle enum types properly. Another subtle difference which we overlooked. Need a unit test (found this while working on dehydra port).

Comment 33

[Vlad Sukhoy]

Attachment: treedydra mac patch (wip)

Comment 34

[Vlad Sukhoy]

Attachment: mac gty patch v1

So basically treehydra builds and passes tests with these patches. Note that gty patch is basically per comment 8 plan b, so gcc won't build with it until we hack that yacc gty parser. Until that one can apply plugin patch, build gcc, build dehydra then apply gty patch and build treehydra.

The patch also contains dehydra modifications to fix various headers issues and aforementioned enum problem.

Comment 35

[Vlad Sukhoy]

Attachment: treedydra mac patch v1

Cleaned up treehydra port mods and added unit test for invisible enum bug in mac dehydra.

Comment 36

[Vlad Sukhoy]

Attachment: unbitrotted treehydra mac v1

Unbitrotted treehydra port.

Comment 37

[(dormant account)]

Comment on attachment 316028 [details] [diff] [review]
unbitrotted treehydra mac v1

>diff -r 0bb98f4cb7a7 configure
>--- a/configure	Wed Apr 16 11:11:25 2008 -0500
>+++ b/configure	Wed Apr 16 11:15:18 2008 -0500
>@@ -120,6 +120,8 @@ if __name__ == "__main__":
>     cflags = ""
>     if os.environ.has_key("CFLAGS"):
>         cflags = os.environ["CFLAGS"]
>+    if platform.system() == 'Darwin':
>+        cflags += ' -fnested-functions'

Ha! I think I'd rather remove that crappy nested function. I figured GCC on all platforms supported those by default.

>     f = open("config.mk", "w")
>     f.write("""GCCDIR=%s
> GCCBUILDDIR=%s
>diff -r 0bb98f4cb7a7 convert_tree.js
>--- a/convert_tree.js	Wed Apr 16 11:11:25 2008 -0500
>+++ b/convert_tree.js	Wed Apr 16 11:15:18 2008 -0500
>@@ -57,7 +57,7 @@ Unit.prototype.toString = function () {
>   var forwards = this.functions.map (function (x) { return x.prefix + x.type + " " + x.name })
>   forwards.push("")
>   var str = forwards.join (";\n");
>-  var bodies = this.functions.map (function (x) { 
>+  var bodies = this.functions.map (function (x) {
>     return "// " + x.comment + "\n"
>       + x.prefix + x.type + " " + x.name + " {\n  " + x.body + "\n}\n";
>   })


>@@ -68,9 +68,9 @@ Unit.prototype.toString = function () {
> 
> function callGetExistingOrLazy (type, name, isAddrOf, cast, isPrimitive, isArrayItem) {
>   if (isAddrOf && !isArrayItem) {
>-    var expr = "&var->" + name 
>+    var expr = "&var->" + name
>     var func = "get_lazy";
>-    return func + " (this, lazy_" + type + ", " 
>+    return func + " (this, lazy_" + type + ", "
>       + expr + ", obj, \"" + name + "\")"
>   }
>   var deref = isAddrOf ? "&" : "";
>@@ -83,7 +83,7 @@ function callGetExistingOrLazy (type, na
>   if (isPrimitive) {
>       return "convert_" + type + "(this, " +  dest + ", " + propValue + ", " + expr + ");";
>   }
>-  return "get_existing_or_lazy (this, lazy_" + type + ", " 
>+  return "get_existing_or_lazy (this, lazy_" + type + ", "
>     + expr + ", " + dest + ", " + propValue + ")"
> }
> 
>@@ -112,7 +112,7 @@ Unit.prototype.saveEnums = function (fna
> Unit.prototype.saveEnums = function (fname) {
>   var constants = ["// generated by convert_tree.js"]
>   for (var name in this.enumValues) {
>-    constants.push ("const " + name + " = new EnumValue(\"" 
>+    constants.push ("const " + name + " = new EnumValue(\""
>                     + name + "\", " + this.enumValues[name] + ")")
>   }
>   write_file (fname, constants.join("\n"))

Not sure what went on above, whitespace changes?

>@@ -123,11 +123,15 @@ Unit.prototype.addEnum = function (field
>   var ls = []
>   ls.push ("jsval v;");
>   ls.push ("switch (var) {");
>+  const enumNames = {}
>   for each (var f in fields) {
>+    this.registerEnumValue (f.name, f.value)
>+    if (enumNames[f.value])
>+      continue;
>+    enumNames[f.value] = f.name
>     ls.push ("case " + f.name + ":");
>     ls.push ("  v = get_enum_value (this, \"" + f.name + "\");")
>     ls.push ("  break;")
>-    this.registerEnumValue (f.name, f.value)
>   }

Good one.

>   ls.push ("default:")
>   ls.push ("  v = JSVAL_NULL;")
>@@ -142,7 +146,7 @@ function callUnion (type, name, unionRes
> function callUnion (type, name, unionResolver) {
>   var obj = "obj_" + name
>   var p1 = "struct JSObject *" + obj + " = dehydra_defineObjectProperty (this, obj, \"" + name + "\");"
>-  return p1 + "\nconvert_" + type + "_union (this, " + unionResolver + ", &var->" + name 
>+  return p1 + "\nconvert_" + type + "_union (this, " + unionResolver + ", &var->" + name
>     + ", " + obj + ")";
> }
> 
>@@ -165,7 +169,7 @@ Unit.prototype.addStruct = function (fie
>       var lls = ["  {", "size_t i;"]
>       lls.push ("char buf[128];")
>       lls.push ("const size_t len = " + f.arrayLengthExpr + ";")
>-      lls.push ("struct JSObject *destArray =  dehydra_defineArrayProperty (this, obj, \"" 
>+      lls.push ("struct JSObject *destArray =  dehydra_defineArrayProperty (this, obj, \""
>                 + f.name + "\", len);")
>       lls.push ("for (i = 0; i < len; i++) {");
>       lls.push ("  sprintf (buf, \"%d\", i);")
>@@ -204,7 +208,7 @@ function getPrefix (aggr) {
>     for (var x = 1; x <= 5; x++) {
>       str += getLine(file, line + x);
>     }
>-    
>+
>     //typedef struct struct_name..means have to use struct on var
>     var bracePos = str.indexOf ("{", pos);
>     var namePos = str.indexOf (aggr.name, pos);
>@@ -226,7 +230,7 @@ function isPointer (type) {
>   return type.isPointer;
> }
> 
>-const tagRegexp = /tag\s*\("([A-Z0-9_]+)"\)/
>+const tagRegexp = /(?:gty:\()?tag\s*\("([A-Z0-9_]+)"\)\)?/

Not sure what went on here. You know a few more regexp tricks that I do, please document what the regexp does.

> // should probly combine attribute extraction into an eval-happy general solution
> function getUnionTag (attributes) {
>   for each (var a in attributes) {
>@@ -256,8 +260,8 @@ function getLengthExpr (attributes) {
>     if (a.name != "user")
>       continue;
>     var m = lengthRegexp.exec (a.value);
>-    if (m) return m[1].replace(percentH_Regexp, "(*var)") 
>-  }  
>+    if (m) return m[1].replace(percentH_Regexp, "(*var)")
>+  }
> }
> 
> const specialRegexp = /special/
>@@ -266,7 +270,7 @@ function isSpecial (attributes) {
>     if (a.name != "user")
>       continue;
>     if (specialRegexp.test(a.value)) return true
>-  }  
>+  }
> }
> 
> const skipRegexp = /skip/
>@@ -275,7 +279,7 @@ function isSkip (attributes) {
>     if (a.name != "user")
>       continue;
>     if (skipRegexp.test(a.value)) return true
>-  }  
>+  }
> }
> 
> const charRegexp = /char/
>@@ -327,7 +331,7 @@ function convert (unit, aggr, unionTopLe
>     if (lengthResults && lengthResults[1] != "u")
>       lengthExpr = lengthResults[1]*1 + 1
>     var cast = undefined
>-    if (m.name == "tree_base::code") {
>+    if (m.name == "tree_base::code" || m.name == "tree_common::code") {
>       type_kind = "enum"
>       type = this.tree_code_type
>       type_name = type.name
>@@ -392,7 +396,7 @@ function convert (unit, aggr, unionTopLe
>         print (m.name + " is special. Skipping...")
>         continue
>       }
>-    } 
>+    }
>     ls.push (new Field (type_name,
>                         name,
>                         tag,
>@@ -413,7 +417,7 @@ function convert (unit, aggr, unionTopLe
>     ret = unit.addEnum (aggr.members.map (function (x) {
>       return {name : x.name, value: x.value}
>     }), stripPrefixRegexp.exec(aggr.name)[1])
>-  } 
>+  }
>   return ret
> }
> 
>diff -r 0bb98f4cb7a7 cp-tree-jsapi-workaround.h
>--- a/cp-tree-jsapi-workaround.h	Wed Apr 16 11:11:25 2008 -0500
>+++ b/cp-tree-jsapi-workaround.h	Wed Apr 16 11:15:18 2008 -0500
>@@ -1,14 +1,22 @@
> /* -*- Mode: C; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> /* A workaround for weird conflict between Apple GCC 4.2 Preview 1's cp-tree.h
>    and SpiderMonkey's jsotypes.h */
>+#if !defined(TREEHYDRA_CONVERT_JS) && !defined(TREEHYDRA_GENERATED)
>+// do not include JS in special and therefore fragile files of treehydra
> #include "jsapi.h"
>+#endif

Looks good.
> 
>+#ifdef JSVAL_OBJECT
>+// workaround only when JS was actually included
> #ifndef uint32
> #define DEHYDRA_DEFINED_uint32
> #define uint32 uint32
> #endif
>+#endif // JSVAL_OBJECT
> #include <cp-tree.h>
>+#ifdef JSVAL_OBJECT
> #ifdef DEHYDRA_DEFINED_uint32
> #undef uint32
> #undef DEHYDRA_DEFINED_uint32
> #endif
>+#endif // JSVAL_OBJECT

This is ever so messy, wish there was a cleaner way of doing this.
I eventually might abstract away the scripting lang from treehydra since that would also be beneficial for cleaning up this mess.

>diff -r 0bb98f4cb7a7 dehydra_plugin.c
>--- a/dehydra_plugin.c	Wed Apr 16 11:11:25 2008 -0500
>+++ b/dehydra_plugin.c	Wed Apr 16 11:15:18 2008 -0500
>@@ -164,6 +164,8 @@ static void process (tree t) {
>     return process_field_decl (t);
>   case TEMPLATE_DECL:
>     return process_template_decl (t);
>+  case CONST_DECL:
>+    return process_type (TREE_TYPE(t));

Good catch. Not sure why this didn't bite me before. Wonder if we are missing any other exciting decls, 
default:
xassert(!DECL_P(...)) sure is tempting here.

>   default:
>     /*error ( "unknown tree element: %s", tree_code_name[TREE_CODE(t)]);*/
>     break;
>diff -r 0bb98f4cb7a7 gcc_compat.h
>--- a/gcc_compat.h	Wed Apr 16 11:11:25 2008 -0500
>+++ b/gcc_compat.h	Wed Apr 16 11:15:18 2008 -0500
>@@ -2,6 +2,7 @@
> #ifndef GCC_COMPAT_H
> #define GCC_COMPAT_H
> 
>+#ifndef TREEHYDRA_GENERATED
> #include <config.h>
> #include <system.h>
> #include <coretypes.h>
>@@ -12,6 +13,7 @@
> #include <tree-iterator.h>
> #include <pointer-set.h>
> #include <toplev.h>
>+#endif
> 
> #ifndef cp_walk_tree_without_duplicates
> #define cp_walk_tree_without_duplicates walk_tree_without_duplicates
>diff -r 0bb98f4cb7a7 gcc_cp_headers.h
>--- a/gcc_cp_headers.h	Wed Apr 16 11:11:25 2008 -0500
>+++ b/gcc_cp_headers.h	Wed Apr 16 11:15:18 2008 -0500
>@@ -16,14 +16,14 @@
> #include <coretypes.h>
> #include <tm.h>
> #include <tree.h>
>+
> #if defined(TREEHYDRA_CONVERT_JS) || defined(TREEHYDRA_GENERATED)
> /* this header conflicts with spidermonkey. sorry for above code */
> #include <basic-block.h>
> #include <tree-flow.h>
>-#include <cp-tree.h>
>-#else
>+#endif
>+
> #include "cp-tree-jsapi-workaround.h"
>-#endif
> #include <cxx-pretty-print.h>
> #include <tree-iterator.h>
> #include <pointer-set.h>
>diff -r 0bb98f4cb7a7 test/enum.cc
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/test/enum.cc	Wed Apr 16 11:15:18 2008 -0500
>@@ -0,0 +1,1 @@
>+enum here { a, b, c=42 };
>diff -r 0bb98f4cb7a7 test/test_enum.js
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/test/test_enum.js	Wed Apr 16 11:15:18 2008 -0500
>@@ -0,0 +1,40 @@
>+// { 'test': 'dehydra', 'input': 'enum.cc', 'output': 'unit_test' }
>+
>+// Test that 'enum' type is passed correctly
>+
>+include('unit_test.js')
>+
>+let r = new TestResults();
>+
>+function EnumTestCase(type) {
>+  this.type = type;
>+}
>+
>+EnumTestCase.prototype = new TestCase();
>+
>+EnumTestCase.prototype.runTest = function () {
>+  let type = this.type;
>+  this.assertEquals(type.kind, 'enum');
>+  this.assertEquals(type.name, 'here');
>+  this.assertEquals(type.members.length, 3);
>+  let m = type.members;
>+  this.assertEquals(m[0].name, 'a');
>+  this.assertEquals(m[0].value, 0);
>+  this.assertEquals(m[1].name, 'b');
>+  this.assertEquals(m[1].value, 1);
>+  this.assertEquals(m[2].name, 'c');
>+  this.assertEquals(m[2].value, 42);
>+}
>+
>+function process_type(type) {
>+  new EnumTestCase(type).run(r);
>+}
>+
>+function input_end() {
>+  if (r.testsRun != 1) {
>+    print('Error: must be 1 tests run, instead ran ' + r.testsRun);
>+  }
>+  else {
>+    r.list();
>+  }
>+}
>\ No newline at end of file
>diff -r 0bb98f4cb7a7 treehydra.c
>--- a/treehydra.c	Wed Apr 16 11:11:25 2008 -0500
>+++ b/treehydra.c	Wed Apr 16 11:15:18 2008 -0500
>@@ -8,7 +8,7 @@
> #include <coretypes.h>
> #include <tm.h>
> #include <tree.h>
>-#include <cp-tree.h>
>+#include "cp-tree-jsapi-workaround.h"
> #include <cxx-pretty-print.h>
> #include <tree-iterator.h>
> #include <pointer-set.h>
>@@ -147,10 +147,17 @@ void lazy_tree_node (Dehydra *this, void
>   enum tree_code code = TREE_CODE (t);
>   i = tree_node_structure(t);
>   /* special case knowledge of non-decl nodes */
>+#ifndef __APPLE_CC__
>+  // no TS_BASE in mac gcc 42 and no easy way to check too
>   convert_tree_node_union (this, TS_BASE, t, obj);
>+#endif
>+#ifdef GIMPLE_TUPLE_P
>   if (!GIMPLE_TUPLE_P (t)) {
>+#endif
>     convert_tree_node_union (this, TS_COMMON, t, obj);
>+#ifdef GIMPLE_TUPLE_P
>   }
>+#endif
>   convert_tree_node_union (this, i, t, obj);
>   /* stuff below is empty for non-decls */
>   if (!DECL_P (t)) return;
>@@ -185,7 +192,7 @@ void convert_int (struct Dehydra *this, 
> 
> void convert_location_t (struct Dehydra *this, struct JSObject *parent,
>                          const char *propname, location_t loc) {
>-  if (loc == UNKNOWN_LOCATION) {
>+  if (loc_is_unknown(loc)) {
>     dehydra_defineProperty (this, parent, propname, JSVAL_VOID);
>     return;
>   }
>@@ -194,7 +201,9 @@ void convert_location_t (struct Dehydra 
>   JSObject *obj = dehydra_defineObjectProperty (this, parent, propname);
>   dehydra_defineStringProperty (this, obj, "file", eloc.file);
>   dehydra_defineProperty (this, obj, "line", INT_TO_JSVAL(eloc.line));
>+#ifndef expand_location
>   dehydra_defineProperty (this, obj, "column", INT_TO_JSVAL(eloc.column));
>+#endif
> }
> /* END Functions for treehydra_generated */
> 
>diff -r 0bb98f4cb7a7 treehydra.js
>--- a/treehydra.js	Wed Apr 16 11:11:25 2008 -0500
>+++ b/treehydra.js	Wed Apr 16 11:15:18 2008 -0500
>@@ -42,7 +42,7 @@ GCCNode.prototype.toString = function ()
> }
> 
> GCCNode.prototype.tree_code = function () {
>-  return this.base.code
>+  return this.base ? this.base.code : this.common.code
> }
> 
> /* Convienient thing along the lines of GENERIC_TREE_OPERAND */
>@@ -266,12 +266,12 @@ function BINFO_BASE_BINFOS (node) {
> 
> var BINFO_TYPE = TREE_TYPE
> 
>-/* This is so much simpler than the C version 
>+/* This is so much simpler than the C version
>  because it merely returns the vector array and lets
> the client for each it*/
> function VEC_iterate (vec_node) {
>   if (vec_node)
>-    return vec_node.base.vec
>+    return vec_node.base ? vec_node.base.vec : vec_node.vec
> }
> 

So if I understand correctly, you are putting in a runtime conditional check for gcc 4.2 differences.
A better solution is to check GCC version once and then override the method by assigning a new one to the prototype. Same might be true for the regexp above, but I'm not completely sure what's going on there.

> function tree_stmt_iterator (ptr, container) {
>diff -r 0bb98f4cb7a7 treehydra_generated.h
>--- a/treehydra_generated.h	Wed Apr 16 11:11:25 2008 -0500
>+++ b/treehydra_generated.h	Wed Apr 16 11:15:18 2008 -0500
>@@ -3,6 +3,7 @@
> #define TREEHYDRA_GENERATED
> /* This file isn't generated, but it's used by the generated one*/
> #include "gcc_cp_headers.h"
>+#include "gcc_compat.h"
> // cleanup some gcc polution in GCC trunk as of Mar 5, 2008
> #ifdef in_function_try_handler
> #undef in_function_try_handler


Overall this is an impressive piece of work. Unfortunately I need to get bug 427661 done first, so this patch will have to bitrot some more, sorry about that. I might try unbitrotting it for you later if it's too much trouble.
Also need to setup a buildbot so we don't keep breaking your environment and to make the OSX port permanent.

Comment 38

[Vlad Sukhoy]

> Not sure what went on above, whitespace changes?
Yeah, the JS emacs mode that I used at the time (Steve Yegge's js2-mode) removes trailing whitespace by default.. Usually I clean that stuff up but in the case of this patch I forgot :(

Will comment on the rest.

Comment 39

[Benjamin Smedberg]

I filed the js2-mode issue as http://code.google.com/p/js2-mode/issues/detail?id=41 last week!

Comment 40

[Vlad Sukhoy]

> Not sure what went on here. You know a few more regexp tricks that I do, please
> document what the regexp does.
IIRC on mac the tag string looks like "gty:(tag(...))" whereas on linux it is just tag(), and the regexp is the greatest common denominator which matches both versions. 

> So if I understand correctly, you are putting in a runtime conditional check
> for gcc 4.2 differences.
Yes, that is correct.

> A better solution is to check GCC version once and then override the method by
> assigning a new one to the prototype.
I am not sure if we have gcc version or platform information available to JS. I agree, that would be a better solution. We could use one of those runtime checks (e.g. no TS_BASE) as a definitive check too.

> I might try unbitrotting it for you later if it's too much trouble.
> Also need to setup a buildbot so we don't keep breaking your environment and to
> make the OSX port permanent.
I am not sure if I will be in the best shape to resolve issues with the port in the coming few days (the MacBook Pro which I used for this work broke and it is possible that I won't have access to a replacement Mac in the short term). At the moment I am using a linux workstation and thus work with linux dehydra.

Comment 41

[Stan Shebs]

I will be happy to take this up, since I'm building latest dehydra daily, and need to have treehydra running too.

Comment 42

[Vlad Sukhoy]

Attachment: treehydra mac v2

Unbitrotted treehydra mac port, addressed review issues. The regexp thing turned out to be unnecessary, probably it creeped in because I was trying to build treehydra when dehydra was built with older spidermonkey, nevermind that part..

Nested functions seem to be needed to compile treehydra_generated.c so this patch only uses the compiler flag for that file and only on a mac. Also this patch eliminates that other nested function.

Comment 43

[Vlad Sukhoy]

Attachment: treehydra mac v2.1

Added proper variable declarations in JS, now using sys.gcc_version to detect Apple compiler, otherwise the same.

Comment 44

[(dormant account)]

Comment on attachment 318245 [details] [diff] [review]
treehydra mac v2.1


>-if platform.system() != 'Darwin':
>-    so_suffix = '.so'
>-    shared_link_flags = '-shared'
>-else:
>-    so_suffix = '.dylib'
>-    shared_link_flags = '-bundle -bundle_loader $(GCCBUILDDIR)/gcc/cc1plus'
>+(dynamic_lib_suffix, shared_link_flags, nested_fn_enabler) = \
>+('.so', '-shared', '') if platform.system() != 'Darwin' \
>+else ('.dylib', '-bundle -bundle_loader $(GCCBUILDDIR)/gcc/cc1plus',
>+      '-fnested-functions')

What is this bundle stuff? Doesn't mac have a dlopen emulation layer now?

Comment 45

[(dormant account)]

Comment on attachment 318245 [details] [diff] [review]
treehydra mac v2.1


>-    if (m.name == "tree_base::code") {
>+    if (m.name == "tree_base::code" || m.name == "tree_common::code") {

I think this is just asking for bitrot. I suggest making a constant tree_code_name or something, and setting it to different values depending on whether dehydra is being run under apple gcc.

Comment 46

[(dormant account)]

Comment on attachment 318245 [details] [diff] [review]
treehydra mac v2.1

>diff -r e50fe9e48c5c dehydra_plugin.c
>--- a/dehydra_plugin.c	Mon Apr 28 16:32:38 2008 -0500
>+++ b/dehydra_plugin.c	Mon Apr 28 16:52:59 2008 -0500
>@@ -164,7 +164,10 @@
>     return process_field_decl (t);
>   case TEMPLATE_DECL:
>     return process_template_decl (t);
>+  case CONST_DECL:
>+    return process_type (TREE_TYPE (t));
>   default:
>+    xassert(!DECL_P (t));
>     /*error ( "unknown tree element: %s", tree_code_name[TREE_CODE(t)]);*/
>     break;
>   }
>diff -r e50fe9e48c5c test/test_enum.js
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/test/test_enum.js	Mon Apr 28 16:52:59 2008 -0500
>@@ -0,0 +1,40 @@
>+// { 'test': 'dehydra', 'input': 'enum.cc', 'output': 'unit_test' }
>+
>+// Test that 'enum' type is passed correctly
>+
>+include('unit_test.js')
>+
>+let r = new TestResults();
>+
>+function EnumTestCase(type) {
>+  this.type = type;
>+}
>+
>+EnumTestCase.prototype = new TestCase();
>+
>+EnumTestCase.prototype.runTest = function () {
>+  let type = this.type;
>+  this.assertEquals(type.kind, 'enum');
>+  this.assertEquals(type.name, 'here');
>+  this.assertEquals(type.members.length, 3);
>+  let m = type.members;
>+  this.assertEquals(m[0].name, 'a');
>+  this.assertEquals(m[0].value, 0);
>+  this.assertEquals(m[1].name, 'b');
>+  this.assertEquals(m[1].value, 1);
>+  this.assertEquals(m[2].name, 'c');
>+  this.assertEquals(m[2].value, 42);
>+}
>+
>+function process_type(type) {
>+  new EnumTestCase(type).run(r);
>+}
>+
>+function input_end() {
>+  if (r.testsRun != 1) {
>+    print('Error: must be 1 tests run, instead ran ' + r.testsRun);
>+  }
>+  else {
>+    r.list();
>+  }
>+}
>\ No newline at end of file

Pushed these and enum.cc

Comment 47

[Stan Shebs]

-bundle_loader is not really necessary; it gives you an extra level of error checking, in that if a symbol is not present in the app that expects to load the bundle, the linker warns you so you don't discover it the hard way at runtime. But since there are multiple compiler executables (cc1, cc1plus, cc1obj, etc) loading the plugin, the flags will want to be changed eventually. In my repo I've been using "-bundle -flat_namespace -undefined suppress" for awhile.

Comment 48

[(dormant account)]

Comment on attachment 318245 [details] [diff] [review]
treehydra mac v2.1

>+#ifndef expand_location
It's not a macro in my GCC, use SOURCE_COLUMN or some other uppercase thing that is more likely to stay a macro.

Comment 49

[(dormant account)]

committed makeEnum changes too, will wait for a respin to commit the rest.

Comment 50

[Vlad Sukhoy]

Attachment: remaining bits of treehydra mac

Remaining review issues, sync'd with stuff from bug 430680. Hopefully, this is it.

Comment 51

[(dormant account)]

pushed.

Comment 52

[Vlad Sukhoy]

Attachment: modd patch for apple gcc 4.2-5531

While verifying the process for plugin-enabled apple-style gcc build with the most recent plugin patch in attachment 312819 [details] [diff] [review] for docs, ran into a problem with the version string change (in "define__GNUC__" it expects ")" after the build number), patching the version string in a somewhat different way fixes the issue.

Comment 53

[(dormant account)]

Vlad/Stan,
I get 

checking size of int... configure: error: cannot compute sizeof (int), 77
See `config.log' for more details.
configure: error: cannot compute sizeof (int), 77
See `config.log' for more details.
make[3]: *** [configure-stage2-libiberty] Error 1
make[3]: *** Waiting for unfinished jobs....
make[3]: *** [configure-stage2-libdecnumber] Error 1
make[2]: *** [stage2-bubble] Error 2
make[1]: *** [all] Error 2
+ exit 1
make: *** [install] Error 1

when i do make install step in http://developer.mozilla.org/en/docs/Dehydra_GCC#Building_GCC_with_plugin_support_on_OSX

Does this mean my OSX 10.4/XCode 2.4.1 are too old? What are the prereqs?

Comment 54

[Vlad Sukhoy]

Hmm. I have OS X 10.5 / XCode 3.0, never tried to build the thing on 10.4

Comment 55

[(dormant account)]

Attachment: config.log for shebs

Comment 56

[(dormant account)]

Comment on attachment 318245 [details] [diff] [review]
treehydra mac v2.1

stale r+