Closed Bug 501609 Opened 12 years ago Closed 12 years ago

Make JS build with VC7.1

Categories

(Core :: JavaScript Engine, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: neil, Assigned: neil)

Details

Attachments

(1 file, 4 obsolete files)

...by working around VC7.1's lack of support for varadic macros and small enums.
* In jstracer.h, use a varadic function instead
* In jstracer.h, only use the enum for VC8 or later
* In Assembler.cpp, don't include commas in macro arguments.
Attached patch Proposed patch (obsolete) — Splinter Review
Assignee: general → neil
Status: NEW → ASSIGNED
Attachment #386233 - Flags: review?(sayrer)
Attached patch supplementary patch (obsolete) — Splinter Review
supplementary patch for Proposed Patch
Attachment #386713 - Flags: review?
Attached patch Combined? patch (obsolete) — Splinter Review
I realised that my original patch had only been tested with VC7.1 debug builds; I spun up a release build and came up with an alternative approach to getting it to build; I don't know why I might have needed slightly different changes.
Attachment #386233 - Attachment is obsolete: true
Attachment #386775 - Flags: review?(sayrer)
Attachment #386233 - Flags: review?(sayrer)
Attachment #386775 - Flags: review?(sayrer) → review?(graydon)
Comment on attachment 386713 [details] [diff] [review]
supplementary patch

>diff -r fd1d917a6ec2 js/src/jsopcode.cpp
>--- a/js/src/jsopcode.cpp	Thu Jun 25 15:13:38 2009 +0200
>+++ b/js/src/jsopcode.cpp	Fri Jul 03 20:35:24 2009 +0800
>@@ -110,7 +110,6 @@
> #undef OPDEF
> };
> 
>-#if defined(DEBUG) || defined(JS_JIT_SPEW)
> /*

What VC-specific code needs this to be unconditional?
 
>diff -r fd1d917a6ec2 js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp	Thu Jun 25 15:13:38 2009 +0200
>+++ b/js/src/jstracer.cpp	Fri Jul 03 20:35:42 2009 +0800
>@@ -358,7 +358,6 @@
> }
> #endif
> 
>-#if defined DEBUG
> static const char*
> getExitName(ExitType type)

Or this?

> #ifdef EXECUTE_TREE_TIMER
>     uint64 cycles = rdtsc() - state.startTime;
>-#elif defined(JS_JIT_SPEW)
>+#else
>     uint64 cycles = 0;
> #endif

Or this?

In general, please provide a context or meaning for each platform-specific adaptation. Such changes may be good or bad, from this distance I can't tell.
Attachment #386713 - Flags: review? → review-
In general the lack of varadic macros in VC7.1 means that whatever you replace them with, typically varadic functions, has to compile, which means that whatever parameters that get passed to them must still be defined. My original patch happened to compile because I had used --enable-debug which turned on all the dependent declarations, but release builds don't have that luxury. However as I hope you noticed I took a slightly different approach to Roy in that I added a VC7.1 conditional to the ifdefs for the affected declarations.
Comment on attachment 386775 [details] [diff] [review]
Combined? patch

>diff -r 01e14fa57337 js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp	Sat Jul 04 03:37:44 2009 +0900
>+++ b/js/src/jstracer.cpp	Sat Jul 04 00:20:53 2009 +0100
> 
>-#if defined DEBUG
>+#if defined(DEBUG) || defined(_MSC_VER) && _MSVC_VER < 1400
> static const char*
> getExitName(ExitType type)

As with previous patch: what exactly is this fixing? And where did the other unexplained bits in the previous patch wind up?

> #ifdef EXECUTE_TREE_TIMER
>     uint64 cycles = rdtsc() - state.startTime;
>-#elif defined(JS_JIT_SPEW)
>+#elif defined(JS_JIT_SPEW) || defined(_MSC_VER) && _MSC_VER < 1400
>     uint64 cycles = 0;
> #endif

Or this? And why *less than* version 1400 in both these hunks. Surely >=? I cannot really guess what's going on here.

>diff -r 01e14fa57337 js/src/jstracer.h
>--- a/js/src/jstracer.h	Sat Jul 04 03:37:44 2009 +0900
>+++ b/js/src/jstracer.h	Sat Jul 04 00:20:53 2009 +0100
> 
>-#ifdef JS_JIT_SPEW
>+#if defined(_MSC_VER) && _MSC_VER < 1400
>+
>+enum LC_TMBits {
>+    /* Output control bits for all non-Nanojit code.  Only use bits 16
>+       and above, since Nanojit uses 0 .. 15 itself. */
>+    LC_TMMinimal  = 1<<16,
>+    LC_TMTracer   = 1<<17,
>+    LC_TMRecorder = 1<<18,
>+    LC_TMPatcher  = 1<<19,
>+    LC_TMAbort    = 1<<20,
>+    LC_TMStats    = 1<<21,
>+    LC_TMRegexp   = 1<<22
>+};

De-duplicate this definition, confine your attention to ifdef'ing only the declaration of the offending variadic macros.

>-#if defined(_MSC_VER) || defined(__GNUC__)
>+#if defined(_MSC_VER) && _MSC_VER >= 1400 || defined(__GNUC__)
> #define USE_TRACE_TYPE_ENUM
> #endif

This part is fine.

> enum JSTraceType_
>-#ifdef _MSC_VER
>+#if defined(_MSC_VER) && _MSC_VER >= 1400
> : int8_t
> #endif

As is this. 

>diff -r 01e14fa57337 js/src/nanojit/Assembler.cpp
>--- a/js/src/nanojit/Assembler.cpp	Sat Jul 04 03:37:44 2009 +0900
>+++ b/js/src/nanojit/Assembler.cpp	Sat Jul 04 00:20:54 2009 +0100
>@@ -820,20 +820,20 @@ namespace nanojit
> 	void Assembler::assemble(Fragment* frag,  NInsList& loopJumps)
> 	{
> 		if (error()) return;	
> 		AvmCore *core = _frago->core();
>         _thisfrag = frag;
> 
> 		// Used for debug printing, if needed
> 		verbose_only(
>-		ReverseLister *pp_init = NULL,
>-            		  *pp_after_sf1 = NULL,
>-			          *pp_after_sf2 = NULL,
>-			          *pp_after_dead = NULL;
>+		ReverseLister *pp_init = NULL;
>+		ReverseLister *pp_after_sf1 = NULL;
>+		ReverseLister *pp_after_sf2 = NULL;
>+		ReverseLister *pp_after_dead = NULL;
>     	)

And this. So .. *mostly* ok, but needs a little clarification / cleanup.
Attachment #386775 - Flags: review?(graydon) → review-
(In reply to comment #5)
> In general the lack of varadic macros in VC7.1 means that whatever you replace
> them with, typically varadic functions, has to compile, which means that
> whatever parameters that get passed to them must still be defined. My original
> patch happened to compile because I had used --enable-debug which turned on all
> the dependent declarations, but release builds don't have that luxury. However
> as I hope you noticed I took a slightly different approach to Roy in that I
> added a VC7.1 conditional to the ifdefs for the affected declarations.

Ok. If these cases are all about faking out the variadic macros (sigh), how about defining a HAVE_VARIADIC_MACROS bit of configury, and changing the ifdefs to reflect that. And possibly putting a comment into cases that are using !defined(HAVE_VARIADIC_MACROS) to point to this bug, and/or mention that the conditional only exists due to the condition-ee being used *in* a variadic macro / function-as-fake-macro argument list.

This is so ghastly; thanks for being patient.
Attached patch MOZ_NO_VARADIC_MACROS (obsolete) — Splinter Review
Like this, perhaps? (I fixed some of the other existing workaround ifdefs too.)
Attachment #386713 - Attachment is obsolete: true
Attachment #386775 - Attachment is obsolete: true
Attachment #388463 - Flags: review?(graydon)
Unfortunately adding MOZ_NO_VARADIC_MACROS to configure.in doesn't work because the headers get included from outside JS, so I came up with this alternative.
Attachment #388463 - Attachment is obsolete: true
Attachment #388658 - Flags: review?(graydon)
Attachment #388463 - Flags: review?(graydon)
Comment on attachment 388658 [details] [diff] [review]
Without configure.in

Ideal! Thanks for all the adaptation. This looks nice and tidy now.
Attachment #388658 - Flags: review?(graydon) → review+
Pushed changeset d190d9b6ccd1 to mozilla-central.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.