Bug 96127 – JSC: Fix some llint C++ interpreter bugs

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

This fixes the bugs reported at https://bugs.webkit.org/show_bug.cgi?id=95749#c8.

==================================================
Csaba Osztrogonac 2012-09-07 08:56:54 PST (-) [reply] 
Created an attachment (id=162783) [details]
LLInt C loop

I'm fighting with LLInt C loop too. I generated the 
LLIntAssembly.h manually and then tried to build JSC.

I had to fix the following opcodes, because GCC suggested parenthesis:
- OFFLINE_ASM_OPCODE_LABEL(op_pre_inc)
- OFFLINE_ASM_OPCODE_LABEL(op_pre_dec)
- OFFLINE_ASM_OPCODE_LABEL(op_post_inc)
- OFFLINE_ASM_OPCODE_LABEL(op_post_dec)
- OFFLINE_ASM_OPCODE_LABEL(op_add)
- OFFLINE_ASM_OPCODE_LABEL(op_sub)

SIGN_BIT32(b-a) --> SIGN_BIT32((b-a))
SIGN_BIT32(b+a) --> SIGN_BIT32((b+a))

I assume it can be fixed in the original code somewhere.

But the bigger problem is the following error messege:
/home/oszi/WebKit/Source/JavaScriptCore/llint/LLIntAssembly.h:125: error: ‘assert’ was not declared in this scope

Have you got any ide what can be the problem and how can we fix it?

==================================================

[Mark Lam]

Created attachment 162800 [details]
Fix.

> SIGN_BIT32(b-a) --> SIGN_BIT32((b-a))
> SIGN_BIT32(b+a) --> SIGN_BIT32((b+a))

This is a bug.  The SIGN_BIT32() macro in llint/LowLevelInterpreter.cpp has been fixed to resolve this.

> But the bigger problem is the following error messege:
> /home/oszi/WebKit/Source/JavaScriptCore/llint/LLIntAssembly.h:125: error: ‘assert’ was not declared in this scope

This issue is resolved by introducing the LLINT_ASSERT() macro which is only used locally in llint/LowLevelInterpreter.cpp.

[Geoffrey Garen]

Comment on attachment 162800 [details]
Fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=162800&action=review

> Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:117
> +#define LLINT_ASSERT(x) llintAssert((x), __FILE__, __LINE__, WTF_PRETTY_FUNCTION, #x)

I think you should just fix the WTF macro. ASSERT is not performance-critical. It's better to have just one ASSERT in the project, which suits all our needs.

[Mark Lam]

Comment on attachment 162800 [details]
Fix.

Will revisit fixing the assert in WTF's ASSERT() macro.

[Mark Lam]

Created attachment 162844 [details]
Fixed WTF's ASSERT() instead.

Not ready for the commit-queue yet.  Need to try out the patch on the EWS to make sure that it doesn't break other ports first.

[Geoffrey Garen]

Comment on attachment 162844 [details]
Fixed WTF's ASSERT() instead.

View in context: https://bugs.webkit.org/attachment.cgi?id=162844&action=review

> Source/WTF/wtf/Assertions.h:247
> +static ALWAYS_INLINE void wtfAssert(bool assertion, const char* file, int line,

ALWAYS_INLINE isn't meaningful here. Inlining is turned of in ASSERT builds. Like I said before, ASSERT is not a performance-relevant code path.

> Source/WTF/wtf/Assertions.h:249
> +                                    const char* function,
> +                                    const char* assertionString)

WebKit style is not to indent lines like this, because it adds extra busy work to later edits to keep the lines equally indented. You can either put "function" and "assertionString" on the same line, indented by one tabstop, or you can put each of assertion, file, line, function, and assertionString on their own lines, indented by one tabstop.

[Filip Pizlo]

Comment on attachment 162844 [details]
Fixed WTF's ASSERT() instead.

View in context: https://bugs.webkit.org/attachment.cgi?id=162844&action=review

>> Source/WTF/wtf/Assertions.h:249
>> +static ALWAYS_INLINE void wtfAssert(bool assertion, const char* file, int line,
>> +                                    const char* function,
>> +                                    const char* assertionString)
> 
> WebKit style is not to indent lines like this, because it adds extra busy work to later edits to keep the lines equally indented. You can either put "function" and "assertionString" on the same line, indented by one tabstop, or you can put each of assertion, file, line, function, and assertionString on their own lines, indented by one tabstop.

I would have done:

static void wtfAssert(
    bool assertion, const char* file, int line,
    const char* function, const char* assertionString)

This works with emacs for me (using the stroustrup cc-mode style).

[Mark Lam]

Created attachment 162895 [details]
Replaced ALWAYS_INLINE with inline, and fixed args indentation.

[WebKit Review Bot]

Comment on attachment 162895 [details]
Replaced ALWAYS_INLINE with inline, and fixed args indentation.

Clearing flags on attachment: 162895

Committed r127938: <http://trac.webkit.org/changeset/127938>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 96166

[Mark Lam]

Created attachment 162925 [details]
Fixed ASSERT() to be C friendly again. Had to give up on "inline" after all.

[Mark Lam]

The changes I made in the latest patch are:
1. change "bool" to "int" so that it's C compliant.
2. renamed wtfAssert() to WTFAssert() to be consistent with other WTF assertion functions, and made it WTF_EXPORT_PRIVATE.
3. Move WTFAssert to wtf/Assertion.cpp.

[Build Bot]

Comment on attachment 162925 [details]
Fixed ASSERT() to be C friendly again. Had to give up on "inline" after all.

Attachment 162925 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13774978

[Mark Lam]

Created attachment 162930 [details]
Windows build needs some love.

[Mark Lam]

Comment on attachment 162930 [details]
Windows build needs some love.

The Windows EWS is happy.  The others should be too (based on the previous patch).  Ready for a review.

[Filip Pizlo]

(In reply to comment #11)
> Created an attachment (id=162925) [details]
> Fixed ASSERT() to be C friendly again. Had to give up on "inline" after all.

Why did you have to give up on inline?

[Mark Lam]

(In reply to comment #16)
> (In reply to comment #11)
> > Created an attachment (id=162925) [details] [details]
> > Fixed ASSERT() to be C friendly again. Had to give up on "inline" after all.
> 
> Why did you have to give up on inline?

Because ASSERT() has to be friendly for C code, and I was told that I can't expect the C compiler to support C99.  Didn't want to assume inline is supported as a modifier.

[Geoffrey Garen]

Comment on attachment 162930 [details]
Windows build needs some love.

Something Sam mentioned today: One advantage of #define is that an ASSERT failure drops into the debugger inside the stack frame that caused the failure. Is there a way to support what you want to do, and still get the debugger to stop in the caller function?

[Mark Lam]

(In reply to comment #18)
> (From update of attachment 162930 [details])
> Something Sam mentioned today: One advantage of #define is that an ASSERT failure drops into the debugger inside the stack frame that caused the failure. Is there a way to support what you want to do, and still get the debugger to stop in the caller function?

In my implementation, it will drop into the WTFAssert(), and you can just go back up to its caller to see the function that failed the assertion.  To me, that gives me equivalent functionality.

[Mark Lam]

Created attachment 163028 [details]
New patch with extra stuff.  See accompanying comment for details.

Changed ASSERT(), and ASSERT_AT() macro to use a #define'd comma expression instead of a function call.  This keeps the folks happy who want assertion failures to break the debugger in the asserting function.

Resolved some extra issues regarding the C++ llint backend that I found while trying to implement the Windows port:
- CLoop::execute()'s bootstrapOpcodeId does not need a default value.  This in turn ensures that the opcode var is always initialized to keep MSVC happy.  It was harmless before, but this is cleaner.
- In LowLevelInterpreter.asm, moved the dispatchAfterCall() call to where it is needed.  For the C_LOOP back-end, this was previously generates unreachable code.
- Placate a MSVC warning for t0, and t1 being uninitialized in CLoop::execute().
- MSVC doesn't like UNUSED_PARAM() for labels. Switched to using the new UNUSED_LABEL() macro.
- offlineasm/generate_offset_extractor.rb was generating code that initialize an unsigned array with int values, and MSVC complained.  Resolved this with a cast in the right places.

Will let the EWS validate this patch first to make sure that other compilers do not complain before I submit this for review.

[Geoffrey Garen]

Comment on attachment 163028 [details]
New patch with extra stuff.  See accompanying comment for details.

View in context: https://bugs.webkit.org/attachment.cgi?id=163028&action=review

EWS builders are happy, and this patch resolves the complaint I heard about ASSERT(), so I'm marking it r+.

> Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:224
> +#if COMPILER(MSVC)

FWIW, other compilers may issue the same complaint eventually.

[WebKit Review Bot]

Comment on attachment 163028 [details]
New patch with extra stuff.  See accompanying comment for details.

Clearing flags on attachment: 163028

Committed r128015: <http://trac.webkit.org/changeset/128015>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.