RFC: improve inlined line numbers, take 2 #16335

JeffBezanson · 2016-05-12T17:52:32Z

This is a rebased and edited version of #14949. To reduce the scope a bit, I changed it to store file names and function names instead of references to LambdaInfos. The goal is to get something mergeable as soon as possible, as this change has several important fixes from @carnaval :

It fixes the formatting of our LLVM debug info and represents inlined functions properly.
It changes the representation of source location info in our IR.
It adds the ability to represent any number of nested inlined functions in backtraces.

I want to get these changes in even if the backtraces are not perfect yet. We can improve it later, either by adding back the LambdaInfo (or Method) references or by keeping a mapping from filename and line number to Methods.

I will merge #15583 into this branch, so we can take advantage of some of this for macro code locations as well.

tkelman · 2016-05-12T18:22:28Z

src/debuginfo.cpp

@@ -759,6 +769,10 @@ extern "C" void jl_register_fptrs(uint64_t sysimage_base, void **fptrs, jl_lambd
    sysimg_fvars_n = n;
 }

+#ifdef _OS_LINUX_
+#include <link.h>


top of file, around L53-54?

JeffBezanson · 2016-05-12T23:58:38Z

I'm not sure how to handle locations from macro expansions. My guess is that there is not good support for the filename magically switching in the middle of a function, so for now I handled it the same way as an inlined function (since that seems to work well), with the inlined function name being macro expansion.

toivoh · 2016-05-13T04:56:00Z

Will there be a way for Julia code examining the AST to figure or which file that lines from a macro expansion came from?

StefanKarpinski · 2016-05-13T13:11:54Z

I'm not sure how to handle locations from macro expansions.

I think ultimately the only way to handle generated code sanely will be to keep generated ASTs and be able to step through them. The julia program should have a command-line argument --keep-source=[yes|no] which defaults to "yes" in interactive mode and defaults to "no" in non-interactive mode.

JeffBezanson · 2016-05-13T13:47:39Z

Will there be a way for Julia code examining the AST to figure or which file that lines from a macro expansion came from?

Yes, there are push_loc and pop_loc for blocks from a different file.

timholy · 2016-05-13T15:18:29Z

Big fan of this effort.

there are push_loc and pop_loc for blocks from a different file.

Awesome! Doesn't yet seem documented in this PR, though. Might need a small section in devdocs or something.

Keno · 2016-05-16T16:48:25Z

src/codegen.cpp

@@ -4286,7 +4277,7 @@ static std::unique_ptr<Module> emit_function(jl_lambda_info_t *lam, jl_llvm_func
        #ifndef LLVM34
        SP = dbuilder.createFunction((DIDescriptor)dbuilder.getCU(),
        #else
-        SP = dbuilder.createFunction(CU,
+        SP = dbuilder.createFunction(topfile,


I'm not sure this works. If the function isn't associated to a compile unit, it might not end up being represented in the binary.

JeffBezanson · 2016-05-16T20:17:12Z

Dev docs added and comments addressed. Should be ready to merge.

tkelman · 2016-05-16T20:39:28Z

test/backtrace.jl

        have_backtrace = true
        break
    end
 end

 @test have_backtrace

-# Test location information for inlined code (ref issues #1334 #12544)


if the results of these all changed, can we change them to test the affirmative of what they should be now, instead of deleting them altogether?

Ah yes, will do.

tkelman · 2016-05-17T03:51:06Z

This breaks building against LLVM 3.3, if we're still concerned with trying to keep that working for testing/perf comparison.
https://gist.github.com/02d8c53f6c9e5962cb738292478646bb

carnaval · 2016-05-17T10:44:44Z

Thanks a lot. I got sidetracked with the memory issues and was starting to lose faith ;). Glad to see a lot of it was salvageable. Allocating our own jit sections was an easy win if we don't care too much about W^X but I'm not sure that'll be the consensus.

JeffBezanson · 2016-05-17T13:28:21Z

Yeah the allocation part is definitely important to work on, but should just be done separately. I suspect it's probably ok to use RWX pages by default, with a switch to disable them.

JeffBezanson · 2016-05-17T21:18:13Z

It would make me happy to remove all the LLVM version ifdefs. If we have ifdefs for some LLVM version, but it's not tested in CI and even most developers never run it, is it really supported? The readability and maintainability gains would be significant.

However currently I'm having trouble building with LLVM 3.3. I get errors about std::unique_ptr and such. There must be another switch I need to flip.

tkelman · 2016-05-17T21:25:40Z

CXXFLAGS=-std=c++11

I agree that it would help clean things up, but 3.7 is still a pretty bad compiler perf regression relative to 3.3 isn't it? Though I see a failure at the end of bootstrap that was introduced by one of the codegen rewrites in March (#15556), so maybe it's already broken and not salvagable.

yuyichao · 2016-05-17T21:27:16Z

You need CXXFLAGS+=' -std=c++0x' to compile with llvm 3.3 with gcc. I've also seen issue about linker confused by '@' in the symbol table though.

JeffBezanson · 2016-05-17T21:29:40Z

Thanks.

3.7 is still a pretty bad compiler perf regression relative to 3.3 isn't it?

Yes it is. But we already decided to make that tradeoff.

tkelman · 2016-05-17T21:40:53Z

On master, as a default build configuration. We don't have every-commit CI for lots of nominally supported configurations, ARM, FreeBSD, etc. We haven't deleted all of the LLVM 3.3 code yet, so the decision of whether or not to keep that a possible option for testing hasn't totally been finalized.

yuyichao · 2016-05-17T21:45:42Z

I believe @vtjnash is testing it on Mac, which doesn't require much special configuration due to clang being permissive about c++11 features and @ not being a special character for the linker. (FWIW g++ 6.1 use gnu++14 by default so the CXXFLAGS setting shouldn't be necessary anymore)

JeffBezanson · 2016-05-18T17:30:31Z

OK, LLVM 3.3 build restored to at least as working as it was before.

- add location info to empty blocks; fixes #15280 - remove file names from non-initial `line` nodes - emit `(meta push_loc filename)` and `(meta pop_loc)` around nested blocks from different files, to handle macros - fix line number of `catch` blocks

vtjnash · 2016-05-20T04:09:30Z

Is this the cause of the new failure in genstdlib?

WARNING: missing docs for signature:

    lookup(pointer::Union{Ptr{Void}, UInt}) -> StackFrame

tkelman · 2016-05-20T04:15:37Z

81d2c46

tkelman reviewed May 12, 2016
View reviewed changes

JeffBezanson force-pushed the jb/linenums2 branch 2 times, most recently from 91a3866 to 748b043 Compare May 12, 2016 23:56

JeffBezanson mentioned this pull request May 12, 2016

WIP: overhaul file name info #15583

Closed

4 tasks

JeffBezanson force-pushed the jb/linenums2 branch from 748b043 to 013d32f Compare May 13, 2016 03:20

JeffBezanson changed the title ~~WIP: improve inlined line numbers, take 2~~ RFC: improve inlined line numbers, take 2 May 13, 2016

ihnorton mentioned this pull request May 13, 2016

Function for getting source location inside string macros #9577

Closed

Keno reviewed May 16, 2016
View reviewed changes

JeffBezanson force-pushed the jb/linenums2 branch 2 times, most recently from 85fb83c to b4e7b6f Compare May 16, 2016 20:13

tkelman reviewed May 16, 2016
View reviewed changes

JeffBezanson force-pushed the jb/linenums2 branch from fd24b5e to 3156e4c Compare May 16, 2016 21:45

JeffBezanson force-pushed the jb/linenums2 branch from 3156e4c to 0ca6cb6 Compare May 18, 2016 17:29

carnaval and others added 4 commits May 18, 2016 16:43

Improve inlined line numbers

a09fd8d

report un-named "inlined" code as being from a macro expansion

c719da7

add back backtrace tests for inlined functions

06d4b2a

JeffBezanson force-pushed the jb/linenums2 branch from 0ca6cb6 to 06d4b2a Compare May 18, 2016 20:44

JeffBezanson merged commit 999f1b5 into master May 19, 2016

JeffBezanson deleted the jb/linenums2 branch May 19, 2016 17:39

vtjnash mentioned this pull request May 21, 2016

stacktraces test failing with no-inline #16501

Closed

KristofferC mentioned this pull request May 25, 2016

Call stack is very confused on error #16578

Closed

JeffBezanson mentioned this pull request May 28, 2016

missing LineNumberNodes #14842

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: improve inlined line numbers, take 2 #16335

RFC: improve inlined line numbers, take 2 #16335

JeffBezanson commented May 12, 2016

tkelman May 12, 2016

JeffBezanson commented May 12, 2016

toivoh commented May 13, 2016

StefanKarpinski commented May 13, 2016

JeffBezanson commented May 13, 2016

timholy commented May 13, 2016

Keno May 16, 2016

JeffBezanson commented May 16, 2016

tkelman May 16, 2016

JeffBezanson May 16, 2016

tkelman commented May 17, 2016

carnaval commented May 17, 2016

JeffBezanson commented May 17, 2016

JeffBezanson commented May 17, 2016

tkelman commented May 17, 2016 •

edited

Loading

yuyichao commented May 17, 2016

JeffBezanson commented May 17, 2016

tkelman commented May 17, 2016

yuyichao commented May 17, 2016

JeffBezanson commented May 18, 2016

vtjnash commented May 20, 2016

tkelman commented May 20, 2016

RFC: improve inlined line numbers, take 2 #16335

RFC: improve inlined line numbers, take 2 #16335

Conversation

JeffBezanson commented May 12, 2016

tkelman May 12, 2016

Choose a reason for hiding this comment

JeffBezanson commented May 12, 2016

toivoh commented May 13, 2016

StefanKarpinski commented May 13, 2016

JeffBezanson commented May 13, 2016

timholy commented May 13, 2016

Keno May 16, 2016

Choose a reason for hiding this comment

JeffBezanson commented May 16, 2016

tkelman May 16, 2016

Choose a reason for hiding this comment

JeffBezanson May 16, 2016

Choose a reason for hiding this comment

tkelman commented May 17, 2016

carnaval commented May 17, 2016

JeffBezanson commented May 17, 2016

JeffBezanson commented May 17, 2016

tkelman commented May 17, 2016 • edited Loading

yuyichao commented May 17, 2016

JeffBezanson commented May 17, 2016

tkelman commented May 17, 2016

yuyichao commented May 17, 2016

JeffBezanson commented May 18, 2016

vtjnash commented May 20, 2016

tkelman commented May 20, 2016

tkelman commented May 17, 2016 •

edited

Loading