485535 - build breaks when JS_HAS_SHARP_VARS is disabled and JS_HAS_XML_SUPPORT enabled

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew Paprocki]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.7) Gecko/2009021910 Firefox/3.0.7
Build Identifier: mozilla-central

there are some conflicting #if checks inside of jsparse.cpp and jsscan.cpp which prevent JS_HAS_XML_SUPPORT from being enabled while JS_HAS_SHARP_VARS is disabled.

Reproducible: Always

Steps to Reproduce:
enable JS_HAS_XML_SUPPORT
disable JS_HAS_SHARP_VARS

Comment 1

[Andrew Paprocki]

Attachment: patch to fix #if checks in jsparse/jsscan

the attached patch adds a few JS_HAS_SHARP_VARS checks around a variable that is only declared when JS_HAS_SHARP_VARS is enabled. also, an #if check for a goto label had a condition for JS_HAS_XML_SUPPORT when JS_HAS_SHARP_VARS code is the only code which actually jumps to it.

Comment 2

[Andrew Paprocki]

Attachment: patch to add #if to jsopcode.tbl

Fixes compilation when JS_HAS_SHARP_VARS is disabled because opcode implementation is inside #if check.

jsopcode.tbl: In function 'JSBool js_Interpret(JSContext*)':
jsopcode.tbl:257: error: label 'L_JSOP_DEFSHARP' used but not defined
jsopcode.tbl:258: error: label 'L_JSOP_USESHARP' used but not defined

Comment 3

[Brendan Eich [:brendan]]

Andrew, I gave you canconfirm and editbugs capabilities so bugs you originate will be NEW, not UNCONFIRMED.

/be

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 369686 [details] [diff] [review]
patch to add #if to jsopcode.tbl

Would prefer all-in-one patch. Also, I'm traveling today so need Igor's help. Or mrbkap's but I thought I'd try Igor. We have worked to reduce ifdef creep over the years.

/be

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 369686 [details] [diff] [review]
patch to add #if to jsopcode.tbl

Argh, I should have read the bug. We do not ifdef jsopcode.tbl. Doing so as this patch does breaks the dense, fixed bytecode numbering. All ops are always defined, and other code must cope.

/be

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 369680 [details] [diff] [review]
patch to fix #if checks in jsparse/jsscan

Idea: let's reduce ifdefs and always include notsharp. But I leave the review to Igor.

/be

Comment 7

[Andrew Paprocki]

Attachment: combined patch to fix disabling JS_HAS_SHARP_VARS

Made a single combined patch.

Instead of adding #if to jsopcode.tbl, instead modify jsinterp.cpp to declare opcodes, but make them simply "goto error;" if JS_HAS_SHARP_VARS is not defined.

This also reduces #if checks in jsparse.cpp as Brendan suggested, always declaring the notsharp variable. Let me know if this is what you had in mind.

Comment 8

[Igor Bukanov]

Comment on attachment 369705 [details] [diff] [review]
combined patch to fix disabling JS_HAS_SHARP_VARS

>diff -r 8bfac66fe41a js/src/jsinterp.cpp
>+          BEGIN_CASE(JSOP_DEFSHARP)
> #if JS_HAS_SHARP_VARS
>-          BEGIN_CASE(JSOP_DEFSHARP)
...
>+#else
>+            goto error;
>+#endif /* JS_HAS_SHARP_VARS */
>           END_CASE(JSOP_DEFSHARP)


This is not the correct way to deal with unsupported opcodes: the patch should not change the JSOP_DEFSHARP and JSOP_USE_SHARP cases here. Instead, when !JS_HAS_SHARP_VARS, it should simply add L_JSOP_DEFSHARP and L_JSOP_USESHARP at the end of the interpreter switch where unsupported bytecode error is reported for JS_THREADED_INTERP. See all those L_something after http://hg.mozilla.org/tracemonkey/file/16b79997f3c6/js/src/jsinterp.cpp#l6879 .

Comment 9

[Andrew Paprocki]

The following code was part of r48470:

# if !JS_HAS_SHARP_VARS
  L_JSOP_DEFSHARP:
  L_JSOP_USESHARP:
  L_JSOP_SHARPINIT:
# endif

...which fixed this issue.

changeset:   48470:9c869e64ee26
user:        Luke Wagner <[e-mail]>
date:        Wed Jul 14 23:19:36 2010 -0700
summary:     Bug 549143 - fatvals