Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

emscripten_log() and emscripten_get_callstack() #1635

Merged
merged 4 commits into from
Dec 21, 2013

Conversation

juj
Copy link
Collaborator

@juj juj commented Sep 18, 2013

Add new functions emscripten_log() in emscripten.h which allows printing out log messages with callstack information, and function emscripten_get_callstack(), which allows programmatically obtaining the current callstack.

The callstack retrieval supports both symbol demangling and file+line+column in .js -> file+line+column in original .cpp mapping through source maps.

I am not completely satisfied about the current requirements that are needed to add demangling and source map support to the build. Namely:

  • User C++ code needs to call __cxa_demangle itself to pin down the function from libcxxabi so that it doesn't get DCE'd. I was not yet able to automate the mechanism to add it in.
  • User build architecture needs to manually add cxa_demangle.cpp to the build, along with a -I directory for libcxxabi.
  • A custom shell file is used to add the source map file as a preload dependency. This requires adding two lines in the existing shell.html, I now put the whole modified shell file in the test directory.
  • Source mapping does not work if executing .js in node, since the source map data is not available there.

Thoughts on what would be the best ways to simplify the above requirements?

jsSymbolName = parts[1];
file = parts[2];
lineno = parts[3];
column = 0; // Firefox doesn't carry column information.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need a comment on that in the source, not just on github

@kripken
Copy link
Member

kripken commented Sep 18, 2013

I suggest we build cxa_demangle standalone into js. Then we would include it in emcc if the user asks for this option. Including could be as a pre-js for example. This would fix the first two issues here.

Regarding the loading of the map file - are source maps text? Then we can do a sync xhr to load them the first time.

Regarding node, we can file a bug. Working in the browser is a great first step.

@juj
Copy link
Collaborator Author

juj commented Sep 26, 2013

I now had a go at building the cxa_demangle.cpp to a .js library. I first tried (in emscripten root directory of this branch):

em++ -s SIDE_MODULE=1 system\lib\libcxxabi\src\cxa_demangle.cpp -Isystem\lib\libcxxabi\include -o lib_cxa_demangle.js
em++ -g tests\emscripten_log\emscripten_log.cpp --pre-js lib_cxa_demangle.js -o emscripten_log.js
node emscripten_log.js

but I suppose --pre-js'ing side modules is not a supported machinery. The result in running that code is that the function ___cxa_demangle is not seen by the main code, since it's inside an unnamed function().

Second I tried to follow the linking guide directly with:

em++ -s SIDE_MODULE=1 system\lib\libcxxabi\src\cxa_demangle.cpp -Isystem\lib\libcxxabi\include -o lib_cxa_demangle.js
em++ -s MAIN_MODULE=1 -g tests\emscripten_log\emscripten_log.cpp -o emscripten_log.js
emlink emscripten_log.js lib_cxa_demangle.js out.js

but that gives an error

Main module: emscripten_log.js
Side module: lib_cxa_demangle.js
Output: out.js
Traceback (most recent call last):
  File "C:\Projects\emsdk_uqm\emscripten\incoming\\emlink.py", line 25, in <modu
le>
    main = AsmModule(main)
  File "C:\Projects\emsdk_uqm\emscripten\incoming\tools\asm_module.py", line 55,
 in __init__
    self.tables = self.parse_tables(self.tables_js)
  File "C:\Projects\emsdk_uqm\emscripten\incoming\tools\asm_module.py", line 243
, in parse_tables
    part = part.split('var ')[1]
IndexError: list index out of range

I added a "if 'var ' not in part: continue" to asm_module.py to try to get past of this, but then I got another error with

Main module: emscripten_log.js
Side module: lib_cxa_demangle.js
Output: out.js
main = ['function() { // Emscripten wrappe']
Traceback (most recent call last):
  File "C:\Projects\emsdk_uqm\emscripten\incoming\\emlink.py", line 28, in <modu
le>
    side.relocate_into(main)
  File "C:\Projects\emsdk_uqm\emscripten\incoming\tools\asm_module.py", line 130
, in relocate_into
    main.tables[table] = self.merge_tables(table, main.tables.get(table), data,
replacements, f_bases, f_sizes)
  File "C:\Projects\emsdk_uqm\emscripten\incoming\tools\asm_module.py", line 260
, in merge_tables
    assert len(main) % 2 == 0
AssertionError

which looks like the code gets confused by comments or whitespace? @kripken, could you take a peek when you have time?

@ngld
Copy link
Contributor

ngld commented Sep 26, 2013

emlink only works with asm.js code...

@kripken
Copy link
Member

kripken commented Oct 2, 2013

Yes, emlink is only for asm.js code - we should error on MAIN_MODULE and SIDE_MODULE if asm is not enabled, I'll add that now.

@kripken
Copy link
Member

kripken commented Oct 11, 2013

Reading the docs on name mangling, it looks pretty trivial: http://en.wikipedia.org/wiki/Name_mangling#Complex_example

Basically _Z[N] then a list of number and a string of that length, then the parameter types - which I'm not sure we need here (N means nested, so a class or namespace). Seems pretty trivial to write our own demangler in 20 lines or so of code, and avoid the complexity of needing to include cxa_demangle etc.

@kripken
Copy link
Member

kripken commented Oct 13, 2013

See last commits on adding stackTrace() and automatic stack demangling on abort().

@kripken
Copy link
Member

kripken commented Oct 14, 2013

I wonder if we shouldn't add a printf option for the stack. So printf("%S - blah blah\n") would print out the stack + the text? Or maybe better to not modify printf and keep it posix compliant, and add a function like you did here?

@jcowles
Copy link

jcowles commented Nov 7, 2013

I'm trying to port some code that relies on __cxa_demangle (among other cxxabi features -- see this thread https://groups.google.com/forum/#!topic/emscripten-discuss/nzlKaLNQFrE).

Should I follow the steps outlined by @juj in the first comment above?

@jcowles
Copy link

jcowles commented Nov 7, 2013

That worked for me, though I'm still curious if I should officially integrate this step into our build system, or if a better solution is available.

@juj
Copy link
Collaborator Author

juj commented Nov 7, 2013

@jcowles : Alon did great work in supporting demangle() from native JS side. It's in my todo list to convert to using that, which will solve the inconveniency with the two first bullet points in the PR message. To get callstacks, the third and fourth bullet points still stand.

Even after this pull request is merged, nothing should break if you already did the same manually in your own codebase. Sorry for not giving this much attention in a while, there's been a lot of higher priority work in the meanwhile.

@juj
Copy link
Collaborator Author

juj commented Dec 19, 2013

Ok, updated this pull request to latest. Thanks azakai for the handwritten demangler, it works beautifully now!

}

// Try to demangle the symbol, but fall back to showing the original JS symbol name if not available.
var cSymbolName = (flags & 32/*EM_LOG_DEMANGLE*/) ? _emscripten_demangle(jsSymbolName) : jsSymbolName;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stackTrace() will by default show both demangled and mangled names, i guess you prefer to just show one at a time?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could just call demangle here, and avoid the dependency on emscripten_demangle (although keeping it as a C function is nice for C code).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I followed the approach of letting user print the version he wants. I did not remove _emscripten_demangle since I also thought at first I had exposed it to emscripten.h, but now notice it was not the case, and it takes in a JS string anyways, so not immediately useful from C code. I'll remove emscripten_demangle function for now - perhaps add that later if it's useful.

…ing out log messages with callstack information, and function emscripten_get_callstack(), which allows programmatically obtaining the current callstack.
…e needed by public use when user wants to emscripten_log with a C callstack. Remove redundant emscripten_demangle function.
@juj
Copy link
Collaborator Author

juj commented Dec 20, 2013

Updated the PR. Also added the bug number to the source.

// Process all lines:
lines = callstack.split('\n');
callstack = '';
var firefoxRe = new RegExp('\\s*(.*?)@(.*):(.*)'); // Extract components of form ' Object._main@http://server.com:4324'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should eventually refactor this to a list of regexes, as we will want to support IE and safari as well.

kripken added a commit that referenced this pull request Dec 21, 2013
emscripten_log() and emscripten_get_callstack()
@kripken kripken merged commit 702cf8f into emscripten-core:incoming Dec 21, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants