xplat: Debugger Support #2056

obastemur · 2016-11-18T20:44:53Z

In addition to Debugger support, this PR also brings the required base for compiling code with script profiler.

Fixes #2058

obastemur · 2016-11-18T21:12:17Z

agarwal-sandeep · 2016-11-18T22:05:12Z

test/Debugger/MultipleContextStack.js

  function Script1Func3() { scriptFunc3(); } \
  function setFunc2(func) { scriptFunc2 = func; } \
  function setFunc3(func) { scriptFunc3 = func; }",
-  "samethread", "Script1.js");


"Script1.js" [](start = 16, length = 12)

Why remove this?

When file name is given, LoadScript is trying to find the full path on xplat and that fails

Then we should fix WScriptJsrt::LoadScript?

Not necessarily. The comment there says; // No fileName provided (WScript.LoadScript()), use dummy "script.js" IMHO the intention there is clean. This particular script file doesn't do that

In debugging it makes a difference as when giving the source we will give Unknown script, you can try that by using debugger annotation dumpSourceList().

Make sense however, for this particular test, it doesn't make any difference. Opening a GH issue to track this instead.

agarwal-sandeep · 2016-11-18T22:31:23Z

lib/Runtime/Base/ScriptContext.cpp

+#if defined(ENABLE_SCRIPT_PROFILING)
        if (attach)
        {
            this->RegisterDebugThunk();


RegisterDebugThunk [](start = 18, length = 18)

We use ProfileThunk under debugger and its set in this function, if we are not calling this where are we setting the thunk?

agarwal-sandeep · 2016-11-18T22:48:16Z

test/Debugger/rlexe.xml

    <default>
      <compile-flags>-debuglaunch -dbgbaseline:JsDiagGetFunctionPosition.js.dbg.baseline</compile-flags>
      <files>JsDiagGetFunctionPosition.js</files>
+      <!-- xplat-todo: enable back [not supported] -->


not supported [](start = 36, length = 13)

Why? JsDiagGetFunctionPosition doesn't seems to have any xplat specific code? and it is used in node-chakracore debugging which seems to be working fine on xplat.

Why? JsDiagGetFunctionPosition doesn't seems to have any xplat specific code? and it is used in node-chakracore debugging which seems to be working fine on xplat.

@agarwal-sandeep Intl is not enabled on xplat.

obastemur · 2016-11-22T00:30:09Z

@agarwal-sandeep ping

agarwal-sandeep · 2016-11-22T01:07:51Z

lib/Common/BackendApi.h

 #define DefaultDeferredParsingThunk Js::JavascriptFunction::DeferredParsingThunk
-#ifdef ENABLE_SCRIPT_PROFILING
+#if defined(ENABLE_SCRIPT_PROFILING) || defined(ENABLE_SCRIPT_DEBUGGING)
 #define ProfileDeferredParsingThunk Js::ScriptContext::ProfileModeDeferredParsingThunk


I don't think we use ProfileDeferredParsingThunk in debugging.

Right, the method itself doesn't receive any call but I've fixed it's impl. for xplat instead of adding even more ifdef checks etc.

agarwal-sandeep · 2016-11-22T01:12:03Z

lib/Runtime/Base/Utf8SourceInfo.cpp

+#endif
            {
                return true;
            }


Whole if should be under #ifdef _WN32, we don't want to retrun true without setting sourceName. and in JsRT there is no sourceName support.

agarwal-sandeep · 2016-11-22T01:19:42Z

lib/Runtime/Base/ScriptContext.cpp

                    __finally
+#endif
                    {
                        threadContext->GetDebugManager()->GetDebuggingFlags()->SetIsBuiltInWrapperPresent(isOrigWrapperPresent);


threadContext->GetDebugManager()->GetDebuggingFlags()->SetIsBuiltInWrapperPresent(isOrigWrapperPresent); [](start = 24, length = 104)

How will this work with DISABLE_SEH?

@agarwal-sandeep We don't have SEH on xplat. Can you please explain more?

I mean if we don't have try finally and CallFunction throws how will we make sure SetIsBuiltInWrapperPresent is restored?

If it throws something that requires SEH in place, we can't restore on xplat.

Please change these __try/__finally to use TryFinally function https://github.com/Microsoft/ChakraCore/blob/master/lib/Common/CommonPal.h#L603.

@jianchun That PAL interface is subject to be removal. No libunwind / seh etc..

@obastemur The TryFinally function I mentioned has nothing to do with libunwind / seh, and why "subject to removal"?

@obastemur This was the TryFinally comment not addressed.

Nevermind; I was referring to something totally unrelated and this one is update to one we have in CommonPAL

agarwal-sandeep · 2016-11-22T01:20:58Z

Have we run crawler and exprgen with this change? We should do private testing before checking in this change.

obastemur · 2016-11-22T13:34:48Z

Have we run crawler and exprgen with this change? We should do private testing before checking in this change.

@agarwal-sandeep I tried not to change anything for non xplat. Could you please point out the places might affect Windows?

agarwal-sandeep · 2016-11-22T22:33:32Z

I understand the sentiment that we haven't touched the code for windows but testing a change before it goes in doesn't harm. plus we have a single click test system at http://onecirro/ where you just provide a private and it does the rest.

ThomsonTan · 2016-11-28T18:15:20Z

bin/ch/jstoc.py

+import os
+
+def print_usage():
+    print "usage: jstoc.py <js file path> <variable name>"


usage: jstoc.py [](start = 11, length = 15)

Output some more help message about what this script does?

ThomsonTan · 2016-11-28T18:17:58Z

lib/Common/CommonDefines.h

 #ifdef _WIN32
 // dep: TIME_ZONE_INFORMATION, DaylightTimeHelper, Windows.Globalization
 #define ENABLE_GLOBALIZATION
 // dep: IDebugDocumentContext


Move this comment together with ENABLE_SCRIPT_DEBUGGING?

obastemur · 2016-12-16T06:38:39Z

Also tested with full project. Test/Debug ..

Oceanswave · 2016-12-21T01:08:38Z

ping

obastemur · 2016-12-21T09:52:18Z

@agarwal-sandeep I got the Crawler result and looks like that one is also good.

obastemur · 2016-12-21T21:53:00Z

ping @jianchun @agarwal-sandeep

agarwal-sandeep · 2016-12-21T22:51:34Z

lib/Runtime/Base/ScriptContext.cpp

        JavascriptMethod origEntryPoint = function->GetFunctionInfo()->GetOriginalEntryPoint();

-        __try
+        __TRY_FINALLY_BEGIN // SEH is not guaranteed, see the implementation


Can we add the test case for returnValue to DebuggerCommon

function foo1() {
return 1;
}
function foo2() {
return 2;
}
function foo() {
var x = 1; /bp:resume('step_over');resume('step_over');locals()/
return foo1() + foo2();
}
foo();
WScript.Attach(foo);

@agarwal-sandeep thanks for the test case, added under the DebuggerCommon. Testing on CI now, lets see how Windows testing is going to react to my baseline file line-endings

@agarwal-sandeep checked other tests we have and saw returnedvaluetests1 is actually doing the same. No reason to introduce this extra test ?

jianchun · 2016-12-21T23:29:31Z

lib/Runtime/Base/ScriptContext.cpp

        JavascriptMethod origEntryPoint = function->GetFunctionInfo()->GetOriginalEntryPoint();

-        __try
+        __TRY_FINALLY_BEGIN // SEH is not guaranteed, see the implementation


__TRY_FINALLY_BEGIN // SEH is not guaranteed, see the implementation [](start = 8, length = 68)

I wonder if I misunderstand something. Does this code need sth. special from SEH? Why do you mention SEH and try to keep __try/__finally on Windows?

@jianchun I also didn't observe any SEH specific code after reading to code again.

2 things I've been trying to to with this PR; 1) do not update any Windows specific code unless it is a bug etc. 2) generally update TRY/FINALLY code to macro.

The second one was out of the scope and we hadn't reach to a consensus there. Updating this implementation very close to original while adding a notice. In case, in the future, if someone expects this part of the code handling SEH, the comment above should be enough to keep the intentions away. In the meantime, adding a macro for future use.

OK, no more issues from me. I haven't reviewed other part, assuming @agarwal-sandeep did.

In addition to Debugger support, this PR also brings the required base for compiling code with script profiler.

agarwal-sandeep · 2016-12-22T22:05:00Z

@obastemur @jianchun I have reviewed debugger specific changes and they look good to me

obastemur · 2016-12-23T15:40:59Z

@agarwal-sandeep @jianchun @ThomsonTan Thanks for the reviews.

@Oceanswave Thanks for opening the issue and follow ups!

Merge pull request #2056 from obastemur:debug In addition to Debugger support, this PR also brings the required base for compiling code with script profiler. Fixes #2058

Oceanswave · 2016-12-23T19:03:39Z

@obastemur thank you for the hard work and sticking through. Greatly appreciate the effort.

msftclas added the cla-already-signed label Nov 18, 2016

obastemur force-pushed the debug branch from 4a4473c to 210a0ab Compare November 18, 2016 21:11

agarwal-sandeep reviewed Nov 18, 2016

View reviewed changes

obastemur force-pushed the debug branch from 210a0ab to 62d1bbd Compare November 18, 2016 22:34

agarwal-sandeep reviewed Nov 18, 2016

View reviewed changes

obastemur force-pushed the debug branch 3 times, most recently from 9341ff8 to 06a2fbc Compare November 19, 2016 14:12

obastemur mentioned this pull request Nov 19, 2016

xplat: ChakraDebug API missing from OSx build #2058

Closed

agarwal-sandeep reviewed Nov 22, 2016

View reviewed changes

obastemur force-pushed the debug branch from 06a2fbc to 7ccb919 Compare November 22, 2016 15:31

ThomsonTan reviewed Nov 28, 2016

View reviewed changes

Oceanswave mentioned this pull request Nov 29, 2016

xplat: Missing ES6 Module functionality in Linux and macOS #2112

Closed

obastemur force-pushed the debug branch 3 times, most recently from 3cc4f2d to 0744052 Compare December 16, 2016 06:32

obastemur force-pushed the debug branch 3 times, most recently from 5ac66cf to c933dc4 Compare December 21, 2016 09:45

obastemur force-pushed the debug branch from c933dc4 to d451d13 Compare December 21, 2016 09:46

agarwal-sandeep reviewed Dec 21, 2016

View reviewed changes

obastemur force-pushed the debug branch 2 times, most recently from 356fa3e to 0113657 Compare December 21, 2016 23:28

jianchun reviewed Dec 21, 2016

View reviewed changes

obastemur force-pushed the debug branch 4 times, most recently from d2e0406 to e321447 Compare December 22, 2016 12:07

xplat: Debugger Support

649f3d2

In addition to Debugger support, this PR also brings the required base for compiling code with script profiler.

obastemur force-pushed the debug branch from e321447 to 649f3d2 Compare December 22, 2016 12:30

chakrabot merged commit 649f3d2 into chakra-core:master Dec 23, 2016

xplat: Debugger Support #2056

xplat: Debugger Support #2056

Uh oh!

Conversation

obastemur commented Nov 18, 2016 • edited by dilijev Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

obastemur commented Nov 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

obastemur commented Nov 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agarwal-sandeep commented Nov 22, 2016

Uh oh!

obastemur commented Nov 22, 2016

Uh oh!

agarwal-sandeep commented Nov 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

obastemur commented Dec 16, 2016

Uh oh!

Oceanswave commented Dec 21, 2016

Uh oh!

obastemur commented Dec 21, 2016

Uh oh!

obastemur commented Dec 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

obastemur Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

obastemur commented Nov 18, 2016 •

edited by dilijev

Loading

obastemur Dec 21, 2016 •

edited

Loading