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

Improve the LLVM IR we generate for trivial functions, especially #[naked] ones. #40367

Merged
merged 2 commits into from
Apr 13, 2017

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Mar 8, 2017

These two small changes fix edef1c/libfringe#68:

  • Don't emit ZST allocas, such as when returning ()
  • Don't emit a branch from LLVM's entry block to MIR's START_BLOCK unless needed
    • That is, if a loop branches back to it, although I'm not sure that's even valid MIR

@rust-highfive
Copy link
Collaborator

r? @nrc

(rust_highfive has picked a reviewer for you, use r? to override)

Copy link
Member

@nagisa nagisa left a comment

Choose a reason for hiding this comment

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

LGTM.

bug!("cannot generate operand from rvalue {:?}", rvalue);

// According to `rvalue_creates_operand`, only ZST
// aggregate rvalues are allowed to be operands.
Copy link
Member

Choose a reason for hiding this comment

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

(debug) assertion would be nice here. It is not unlikely for somebody to edit some interaction between some rvalue_creates_operand and this function in a wrong way and an assertion here would catch it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had one! It's now in OperandRef::new_zst.

@eddyb
Copy link
Member Author

eddyb commented Mar 8, 2017

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Mar 8, 2017

📌 Commit df60044 has been approved by nagisa

@bors
Copy link
Contributor

bors commented Mar 9, 2017

🔒 Merge conflict

@alexcrichton
Copy link
Member

@bors: retry

arielb1 pushed a commit to arielb1/rust that referenced this pull request Mar 9, 2017
Improve the LLVM IR we generate for trivial functions, especially #[naked] ones.

These two small changes fix edef1c/libfringe#68:
* Don't emit ZST allocas, such as when returning `()`
* Don't emit a branch from LLVM's entry block to MIR's `START_BLOCK` unless needed
  * That is, if a loop branches back to it, although I'm not sure that's even valid MIR
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 10, 2017
Improve the LLVM IR we generate for trivial functions, especially #[naked] ones.

These two small changes fix edef1c/libfringe#68:
* Don't emit ZST allocas, such as when returning `()`
* Don't emit a branch from LLVM's entry block to MIR's `START_BLOCK` unless needed
  * That is, if a loop branches back to it, although I'm not sure that's even valid MIR
@alexcrichton
Copy link
Member

@bors: r=nagisa

@bors
Copy link
Contributor

bors commented Mar 10, 2017

📌 Commit d8ca084 has been approved by nagisa

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 10, 2017
Improve the LLVM IR we generate for trivial functions, especially #[naked] ones.

These two small changes fix edef1c/libfringe#68:
* Don't emit ZST allocas, such as when returning `()`
* Don't emit a branch from LLVM's entry block to MIR's `START_BLOCK` unless needed
  * That is, if a loop branches back to it, although I'm not sure that's even valid MIR
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 11, 2017
Improve the LLVM IR we generate for trivial functions, especially #[naked] ones.

These two small changes fix edef1c/libfringe#68:
* Don't emit ZST allocas, such as when returning `()`
* Don't emit a branch from LLVM's entry block to MIR's `START_BLOCK` unless needed
  * That is, if a loop branches back to it, although I'm not sure that's even valid MIR
arielb1 pushed a commit to arielb1/rust that referenced this pull request Mar 11, 2017
Improve the LLVM IR we generate for trivial functions, especially #[naked] ones.

These two small changes fix edef1c/libfringe#68:
* Don't emit ZST allocas, such as when returning `()`
* Don't emit a branch from LLVM's entry block to MIR's `START_BLOCK` unless needed
  * That is, if a loop branches back to it, although I'm not sure that's even valid MIR
@arielb1
Copy link
Contributor

arielb1 commented Mar 11, 2017

Eh, @bors r-

Better that @eddyb add a tester for i686-gnu-noopt.

Test failure:

---- [debuginfo-gdb] debuginfo-gdb/macro-stepping.rs stdout ----
	NOTE: compiletest thinks it is using GDB without native rust support
NOTE: compiletest thinks it is using GDB version 7011001

error: line not found in debugger output: [...]#inc-loc1[...]

@bors
Copy link
Contributor

bors commented Mar 20, 2017

☔ The latest upstream changes (presumably #39628) made this pull request unmergeable. Please resolve the merge conflicts.

@eddyb
Copy link
Member Author

eddyb commented Mar 21, 2017

Looks like -noopt isn't needed, this repros:

./x.py test --stage 1 src/test/debuginfo --test-args macro-stepping --target i686-unknown-linux-gnu

@eddyb
Copy link
Member Author

eddyb commented Apr 12, 2017

I've gradually transformed the LLVM IR from Rust to match the LLVM IR from Clang.
This is what I am left with, test.c.ll working as expected, while test.rs.ll not:

--- test.c.ll   2017-04-12 15:58:31.236264978 +0000
+++ test.rs.ll  2017-04-12 15:59:02.302421857 +0000
@@ -29 +29 @@
-!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 3.9.1 (tags/RELEASE_391/final)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "rustc version 1.18.0-dev (57ca703dc 2017-04-11)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)

EDIT: Shorter diff, !llvm.ident does not appear relevant.
EDIT2: GDB harcodes startswith (COMPUNIT_PRODUCER (cust), "clang ").

@eddyb
Copy link
Member Author

eddyb commented Apr 12, 2017

So it turns out that LLVM produces this (for a main that starts with a call):

main:
        .loc    1 5 0
        pushl   %ebp
        movl    %esp, %ebp
        .loc    1 6 5 prologue_end
        subl    $8, %esp
        calll   included

GDB is capable of determining that the last instruction in the prologue is subl.
However, the .loc is before that instruction. If the .loc is moved/duplicated to after subl (i.e. just before calll), then GDB will properly break-on-line (6) at the calll instruction.

@eddyb
Copy link
Member Author

eddyb commented Apr 12, 2017

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Apr 12, 2017

📌 Commit 9b5c577 has been approved by nagisa

@bors
Copy link
Contributor

bors commented Apr 12, 2017

⌛ Testing commit 9b5c577 with merge 3ee3088...

@bors
Copy link
Contributor

bors commented Apr 12, 2017

💔 Test failed - status-travis

@eddyb
Copy link
Member Author

eddyb commented Apr 12, 2017

@bors retry

  • OpenSSL on OSX failed to compile?

@bors
Copy link
Contributor

bors commented Apr 12, 2017

⌛ Testing commit 9b5c577 with merge 8e25752...

@bors
Copy link
Contributor

bors commented Apr 13, 2017

💔 Test failed - status-travis

@alexcrichton
Copy link
Member

alexcrichton commented Apr 13, 2017 via email

@bors
Copy link
Contributor

bors commented Apr 13, 2017

⌛ Testing commit 9b5c577 with merge 1470a1a...

@bors
Copy link
Contributor

bors commented Apr 13, 2017

💔 Test failed - status-travis

@arielb1
Copy link
Contributor

arielb1 commented Apr 13, 2017

Musl segfault:

@bors retry

@bors
Copy link
Contributor

bors commented Apr 13, 2017

⌛ Testing commit 9b5c577 with merge 43ef63d...

bors added a commit that referenced this pull request Apr 13, 2017
Improve the LLVM IR we generate for trivial functions, especially #[naked] ones.

These two small changes fix edef1c/libfringe#68:
* Don't emit ZST allocas, such as when returning `()`
* Don't emit a branch from LLVM's entry block to MIR's `START_BLOCK` unless needed
  * That is, if a loop branches back to it, although I'm not sure that's even valid MIR
@bors
Copy link
Contributor

bors commented Apr 13, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nagisa
Pushing 43ef63d to master...

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.

Crash with the example from the readme
7 participants