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

Add a combined source without #line directives into the dist package #363

Merged
merged 5 commits into from
Nov 4, 2015

Conversation

svaarala
Copy link
Owner

Related to #362. Prototype how duktape.c would work without #line directives:

  • Add optparse to combine_src.py
  • Add a --line-directives option to combine_src.py
  • Emit a file/line metadata file into dist/src/metadata.json
  • Disable line directives in dist combine_src.py step
  • Add an utility to resolve a combined source line into original file/line using dist/src/metadata.json
  • Drop #line directives, or provide both combined source files? => include both, as both variants have relevant use cases
  • Dist documentation changes
  • Other documentation changes; website note and point to wiki?

@svaarala svaarala added this to the v1.4.0 milestone Sep 17, 2015
@svaarala
Copy link
Owner Author

The file/line metadata map is useful for Duktape development: with a combined source traceback line it can resolved to an original source file/line, e.g.:

$ python util/resolve_combined_lineno.py dist/src/metadata.json 12345
duktape.c:12345 -> duk_builtins.c:793

@svaarala
Copy link
Owner Author

I'll give a few days before merging this so that if anyone has a strong opinion one way or another they'll have some time to comment.

The concrete impacts of this change are:

  • Debuggers will be less confused about file/line and source file mappings. The debugger will step through the combined source and not the individual sources as e.g. gdb did before.
  • Some packaging tools (see Why is duktape.c distributed with '#line' cpp entries? #362) should work better without changes.
  • Error tracebacks will contain duktape.c and a combined line number rather than uncombined source file/line.

To illustrate the traceback issue, before the change:

$ ./duk
((o) Duktape 1.3.99 (v1.3.0-45-g22a33b2-dirty)
duk> decodeURIComponent('%ff')
URIError: invalid input
    duk_bi_global.c:343
    decodeURIComponent  native strict preventsyield
    global input:1 preventsyield

After the change:

$ ./duk
((o) Duktape 1.3.99 (v1.3.0-42-ge19dec5-dirty)
duk> decodeURIComponent('%ff')
URIError: invalid input
    dist/src/duktape.c:29866
    decodeURIComponent  native strict preventsyield
    global input:1 preventsyield

$ python util/resolve_combined_lineno.py dist/src/metadata.json 29866
duktape.c:29866 -> duk_bi_global.c:343

The impact for troubleshooting is that if the Duktape source file/line matters, you'll need to figure out the uncombined file/line from the combined line somehow. The pull adds a utility to do that, and adds the necessary metadata into the dist, so that this will be relatively easy at least for official releases.

@fatcerberus
Copy link
Contributor

By the way MSVC behaves similarly to gdb, which threw me off at first: it won't step into duktape.c but instead prompts for the location of the original source files. Normally I don't mind but it can become an issue when prototyping changes directly in the combined source as then the two copies don't match.

On the other hand, it adds an extra level of indirection when deciphering debug prints. So there are pros and cons, and I'm not really sure which way is better.

@svaarala
Copy link
Owner Author

For application programming the impact in tracebacks should be mostly minimal: tracebacks have a function .name too (decodeURIComponent() above) which usually identifies the relevant internal function quite well for troubleshooting.

With lightfuncs it's a bit different because the virtual lightfunc .name just contains a raw pointer and flags but no function name. This mostly matters when using DUK_OPT_LIGHTFUNC_BUILTINS because then almost all built-ins will be lightfuncs.

@fatcerberus
Copy link
Contributor

Hm, debugging may actually be hindered by this: It seems MSVC doesn't like source files with more than 64k lines:
https://connect.microsoft.com/VisualStudio/feedback/details/728010/visual-studio-10-c-debugger-doesnt-work-on-files-with-64k-lines

@svaarala
Copy link
Owner Author

That sounds vaguely familiar. So, without adding both combined source variants we'll have to choose between either:

  • Causing issues with some packaging tools
  • Causing issues with MSVC debugger

We can add both file variants into the distributable if that's the only workable option though. It's just a bit awkward to explain. Concretely this would mean either adding a third source directory (so e.g. src/, src-no-line-directive/, and src-separate/, maybe abbreviated) or add a second file into src/ (e.g. src/duktape.c-no-line-directive or src/duktape-no-line-directive.c).

@glima
Copy link

glima commented Oct 16, 2015

No consensus here yet? :)

@svaarala
Copy link
Owner Author

Any choice will lead to grumbling so I've yet to determine the path of least grumbling ;)

I'll try to get this merged soonish, I've been busy with some other stuff.

@svaarala
Copy link
Owner Author

svaarala commented Nov 3, 2015

Ok, so as a compromise the pull now provides the following dist source structure:

  • src/duktape.c: current single source version with #line directives
  • src-noline/duktape.c: variant without #line directives, directory also contains headers etc
  • src-separate/: current separate source version

The dist README has this note:

* ``src-noline/``: contains a variant of ``src/duktape.c`` with no ``#line``
  directives which is preferable for some users.  See discussion in
  https://github.com/svaarala/duktape/pull/363.

Hopefully this is a reasonable compromise :)

@svaarala svaarala force-pushed the remove-combined-src-line-directives branch from 2b9b5cb to 830b8e1 Compare November 3, 2015 23:14
@fatcerberus
Copy link
Contributor

This seems reasonable. I like that you chose to put it in a separate directory, keeps things neat.

@svaarala
Copy link
Owner Author

svaarala commented Nov 3, 2015

Yeah, I thought it best not to change the src/ directory; someone may be building with the wildcard src/*.c so having two .c files would have been an unwelcome surprise.

@svaarala svaarala force-pushed the remove-combined-src-line-directives branch from 8160d08 to ffcf499 Compare November 4, 2015 11:12
@svaarala svaarala changed the title Remove combined source #line directives Add a combined source without #line directives into the dist package Nov 4, 2015
svaarala added a commit that referenced this pull request Nov 4, 2015
…ives

Add a combined source without #line directives into the dist package
@svaarala svaarala merged commit 1529bec into master Nov 4, 2015
@svaarala svaarala deleted the remove-combined-src-line-directives branch November 4, 2015 11:21
@glima
Copy link

glima commented Nov 4, 2015

Thanks, everyone. I'll be updating for the next release on Soletta soon.

@glima
Copy link

glima commented Nov 12, 2015

I'm a bit confused -- are there any releases contemplating this change yet?

@fatcerberus
Copy link
Contributor

It's in master, which means it will be in the next Duktape release (1.4.0).

@glima
Copy link

glima commented Nov 12, 2015

Thanks a lot :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants