Bug 96127

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.

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.

Comment on attachment 162800 [details] Fix. Will revisit fixing the assert in WTF's ASSERT() macro.

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.

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.

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).

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

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>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by 96166

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

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.

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

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

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.

(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?

(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.

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?

(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.

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.

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.

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>

All reviewed patches have been landed. Closing bug.