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

Always generate line numbers debug information #3831

Conversation

ysbaddaden
Copy link
Contributor

Adds a Crystal::Debug flags enum to select the kinds of debug information to generate, and alters the build flags:

  • default: only generate line numbers debug info (including function names);
  • --debug: generate all debug info;
  • --no-debug: don't generate any debug info.

For example, when compiling the crystal compiler, I see a 12% file increase for line numbers vs 45% for the full debug info (compile units, variables, ...)

  • default: 26MB
  • --debug: 34MB
  • --no-debug: 23MB

@ysbaddaden ysbaddaden force-pushed the enhancement-add-line-numbers-to-all-builds-by-default branch from b1d0a11 to bb22447 Compare January 3, 2017 20:27
@RX14
Copy link
Contributor

RX14 commented Jan 3, 2017

I think this is a good default, however I would make --no-debug be the default with --release.

@ysbaddaden ysbaddaden force-pushed the enhancement-add-line-numbers-to-all-builds-by-default branch from bb22447 to df3fbb3 Compare January 3, 2017 21:01
@ysbaddaden
Copy link
Contributor Author

Why? LLVM means to have the best debugability even with aggresive optimizations. This adds line numbers to backtraces, which may lead to better reports.

@ysbaddaden
Copy link
Contributor Author

Travis fails to compile specs on x86_64 linux ... while it works perfectly for me, and the error should happen on other platforms, too 😭 is the docker image outdated?

@asterite
Copy link
Member

asterite commented Jan 3, 2017

@ysbaddaden Yes, they are outdated. @jhass used to keep them up to date. Tomorrow I'll try to create docker images based on those ones and then use them in travis.

@asterite
Copy link
Member

asterite commented Jan 3, 2017

@ysbaddaden Actually, it seems the build is automatic. I don't know why it failed. I'll try downloading the image and checking if the none? method is there, or what's wrong...

@asterite
Copy link
Member

asterite commented Jan 3, 2017

Hmmm...

$ docker run -it --rm jhass/crystal-build-x86_64
root@c184614d3dcd:/# crystal version
Crystal 0.20.0 [b0cc6f7] (2016-11-22)

😕

@ysbaddaden ysbaddaden force-pushed the enhancement-add-line-numbers-to-all-builds-by-default branch from e189db6 to df3fbb3 Compare January 4, 2017 09:50
@asterite
Copy link
Member

asterite commented Jan 4, 2017

I'd try to make this work without relying on Crystal 0.20.3. Maybe there was a problem with the deb package for x84-64, I don't know. But after I release 0.20.4 that should be fixed. If this can be implemented without none? then it will go into the 0.20.4 release, otherwise I'll merge it after the next release.

@ysbaddaden
Copy link
Contributor Author

I tried to add a None member manually, but it then failed horribly on all platforms on Travis. No idea what happened again. Let's skip a release.

@asterite
Copy link
Member

asterite commented Jan 4, 2017

@ysbaddaden I'm not sure adding a None member will work. One last thing you can try is doing enum.value == 0, which should be equivalent and should compile.

@ysbaddaden
Copy link
Contributor Author

I restarted the travis job. Let's see!

@ysbaddaden
Copy link
Contributor Author

@asterite green at last!

@ysbaddaden
Copy link
Contributor Author

@asterite merge?

@spalladino spalladino merged commit 7865abf into crystal-lang:master Jan 10, 2017
@spalladino
Copy link
Contributor

LGTM, merged. Thanks again @ysbaddaden!

@bcardiff bcardiff added this to the Next milestone Jan 11, 2017
@0x1eef
Copy link

0x1eef commented Jan 12, 2017

@ysbaddaden i think it would be good if --release were to imply --no-debug because the binary will be smaller. --release targets a binary that could be distributed to a client or customer, so i don't think including debug information is that useful, and reduced binary size is a plus if you are offering downloads and so on.

@ysbaddaden ysbaddaden deleted the enhancement-add-line-numbers-to-all-builds-by-default branch January 12, 2017 09:38
@ysbaddaden
Copy link
Contributor Author

  • A 12% increase in binary size is negligible (some KB, a few MB at worst).
  • LLVM has good support for debug even with aggressive optimizations.
  • Line numbers in backtraces make better crash reports.

If you really want to shave a few KB off your binaries, at the expense of harder to decipher crash reports, you can pass --no-debug to change this default, then strip your binary.

@asterite
Copy link
Member

@ysbaddaden This PR made the compiler not be able to reuse previous compilations, ever.

The main problem is that when code is inside a macro, to be able to debug it we generate a file named "macro#{some_id}.cr" and put that inside the debug info. The code for that is here. This some_id changes between compilations, right now it's file.object_id where file is VirtualFile.

Now, the culprit is not this PR but the way debug info is generated for macros, which was like that before this PR. Now it's more noticeable because debug info is always on (this is something that I personally like).

I'm not exactly sure how to solve this. Some ideas:

  1. Not to emit debug info for macros, though I guess that will break some things.
  2. Generate a single macros.cr file containing all macro expansions for the current program, but then line numbers need to be adjusted accordingly, and it's possible that if we change some code around then the order of execution of macros will change and then reusing a previous compilation won't be possible.
  3. Only with Cyrstal::Debug::LineNumbers use the original_filename of the VirtualFile (through Location) though that will have the line numbers wrong when debugging or at exceptions, but in any case line numbers for backtraces where macros appear aren't very useful or informative.
  4. Any other solution?

I think 3 is the easiest/fastest things to do to "fix" this (maybe even do it always, because stepping into macro code might not be very useful either), but I don't know what the general solution is. Macros are hard...

One quick way to notice if the compiler reuses a previous compilation is to do pp changed after this line. The second compilation of a same program should give all false. But with programs that use macros (basically, every program) then it always gives true now. I noticed this because compiling a program with an HTTP::Server in it was always slow. Previously it was only slow the first time. This is because HTTP::StaticFileHandler uses ECR, and that executes a macro run that compiles a program in release mode. The idea is that it compiles it in release mode because the second compilation will find it in the cache (the semantic phase is still done, but that's usually fast for macro run programs, though in any case I want to optimize that further by storing last modified dates of files or something like that). But now it won't find that in the cache, so the ECR/process program is compiled over and over, in release mode. An improvement we can do here is to compile macro run programs with Crystal::Debug::None, but that's just a hack, regular programs will still suffer from slow(er) compilations.

@asterite
Copy link
Member

@ysbaddaden I'll soon send a PR to "fix" this, don't worry about a solution :-)

@ysbaddaden
Copy link
Contributor Author

Great, because I really didn't have one... except maybe using a digest of the macro contents, instead of a random identifier.

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.

6 participants