Skip to content

Avoid unnecessary allocas for indirect function arguments - #44573

Merged
bors merged 1 commit into
rust-lang:masterfrom
dotdash:dbg_bloat
Oct 18, 2017
Merged

Avoid unnecessary allocas for indirect function arguments#44573
bors merged 1 commit into
rust-lang:masterfrom
dotdash:dbg_bloat

Conversation

@dotdash

@dotdashdotdash commented Sep 14, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor

The extra alloca was only necessary because it made LLVM implicitly
handle the necessary deref to get to the actual value. The same happens
for indirect arguments that have the byval attribute. But the Rust ABI
does not use the byval attribute and so we need to manually add the
deref operation to the debuginfo.

@dotdash
dotdash requested a review from eddybSeptember 14, 2017 20:16
@rust-highfive

rust-highfive commented Sep 14, 2017

Copy link
Copy Markdown
Contributor

r? @nikomatsakis

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

@eddyb

eddyb commented Sep 14, 2017

Copy link
Copy Markdown
Contributor

LGTM. r? @michaelwoerister

@alexcrichtonalexcrichton added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 14, 2017
@michaelwoerister

michaelwoerister commented Sep 15, 2017

Copy link
Copy Markdown
Member

@bors r+

Thanks @dotdash!

@bors

bors commented Sep 15, 2017

Copy link
Copy Markdown
Collaborator

📌 Commit ed7a7cf has been approved by michaelwoerister

@michaelwoeristermichaelwoerister added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 15, 2017
TimNN added a commit to TimNN/rust that referenced this pull request Sep 17, 2017
@TimNN
Rollup merge ofrust-lang#44573- dotdash:dbg_bloat, r=michaelwoerister
353d501
Avoid unnecessary allocas for indirect function arguments
The extra alloca was only necessary because it made LLVM implicitly
handle the necessary deref to get to the actual value. The same happens
for indirect arguments that have the byval attribute. But the Rust ABI
does not use the byval attribute and so we need to manually add the
deref operation to the debuginfo.
@TimNNTimNN mentioned this pull request Sep 17, 2017
Closed
@TimNN

TimNN commented Sep 17, 2017

Copy link
Copy Markdown
Contributor

This likely caused rollup #44649 to fail with the following error on x86_64-gnu-debug:

[00:27:41] rustc: /checkout/src/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp:295: void llvm::DwarfExpression::addFragmentOffset(const llvm::DIExpression*): Assertion `FragmentOffset >= OffsetInBits && "overlapping or duplicate fragments"' failed.
[00:27:41] error: Could not compile `std`.

(x86_64-gnu-debug succeeded in rollup #44652, who's only difference to #44649 was that this PR was excluded)

@bors

bors commented Sep 17, 2017

Copy link
Copy Markdown
Collaborator

⌛ Testing commit ed7a7cf with merge ee179583d630a07cf0009739c6edbc61ada961a8...

@arielb1

arielb1 commented Sep 17, 2017

Copy link
Copy Markdown
Contributor

@bors r-

@arielb1

arielb1 commented Sep 17, 2017

Copy link
Copy Markdown
Contributor

@bors retry

@arielb1

arielb1 commented Sep 17, 2017

Copy link
Copy Markdown
Contributor

Turns out leaving LLVM assertions disabled isn't a good idea when working with LLVM.

@michaelwoerister

michaelwoerister commented Sep 18, 2017

Copy link
Copy Markdown
Member

Is there anything we can do about this? Or does this just not work in the general case?

@dotdash

dotdash commented Sep 19, 2017

Copy link
Copy Markdown
ContributorAuthor

Looks like an LLVM bug which is only triggered by this change. The jump threading pass duplicates an dbg.declare intrinsic, causing the same fragment to be declared twice and triggering the assertion. I asked on the LLVM ML about it.

http://lists.llvm.org/pipermail/llvm-dev/2017-September/117575.html

@eddyb

eddyb commented Sep 19, 2017

Copy link
Copy Markdown
Contributor

@dotdash It wouldn't be the first time LLVM mangles non-trivial debuginfo annotations.

@michaelwoerister

michaelwoerister commented Sep 19, 2017

Copy link
Copy Markdown
Member

@dotdash ❤️

@arielb1arielb1 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 19, 2017
@aidanhs

aidanhs commented Sep 28, 2017

Copy link
Copy Markdown
Contributor

For triage clarity, where is this up to? Is there a patch making its way upstream (and if so, is there a link to the LLVM phabricator so we can track it)?

@dotdash

dotdash commented Oct 4, 2017

Copy link
Copy Markdown
ContributorAuthor

Sorry, I've been unable to get back to this earlier. Upstream patch is at https://reviews.llvm.org/D38540

@dotdashdotdash mentioned this pull request Oct 10, 2017
Merged
@dotdash

dotdash commented Oct 11, 2017

Copy link
Copy Markdown
ContributorAuthor

Updated to include the necessary LLVM update and added a second commit to avoid even more copies and fix a debug info oddity/bug.

@dotdash
dotdashforce-pushed the dbg_bloat branch 3 times, most recently from b18ed29 to 43c4589CompareOctober 13, 2017 14:07
@bors

bors commented Oct 14, 2017

Copy link
Copy Markdown
Collaborator

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

@dotdash

dotdash commented Oct 16, 2017

Copy link
Copy Markdown
ContributorAuthor

Rebased and backed out the commit that was failing earlier. Should be good to go now.

@kennytmkennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 16, 2017
eddyb
eddyb reviewed Oct 18, 2017
View reviewed changes
Comment threadsrc/rustllvm/llvm-rebuild-trigger Outdated

@eddybeddybOct 18, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you need to change this?

@arielb1arielb1Oct 18, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There was an LLVM rebuild race.

@arielb1

arielb1 commented Oct 18, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor

r+ with the llvm rebuild trigger removed - it is not needed any more because someone else updated our LLVM.

@kennytmkennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 18, 2017
@michaelwoerister

michaelwoerister commented Oct 18, 2017

Copy link
Copy Markdown
Member

👍

@dotdash
Avoid unnecessary allocas for indirect function arguments
6bfecd4
The extra alloca was only necessary because it made LLVM implicitly
handle the necessary deref to get to the actual value. The same happens
for indirect arguments that have the byval attribute. But the Rust ABI
does not use the byval attribute and so we need to manually add the
deref operation to the debuginfo.
@dotdash

dotdash commented Oct 18, 2017

Copy link
Copy Markdown
ContributorAuthor

@bors r=arielb1

@bors

bors commented Oct 18, 2017

Copy link
Copy Markdown
Collaborator

📌 Commit 6bfecd4 has been approved by arielb1

@kennytmkennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 18, 2017
@bors

bors commented Oct 18, 2017

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 6bfecd4 with merge fc208bb...

bors added a commit that referenced this pull request Oct 18, 2017
@bors
Auto merge of#44573- dotdash:dbg_bloat, r=arielb1
fc208bb
Avoid unnecessary allocas for indirect function arguments
The extra alloca was only necessary because it made LLVM implicitly
handle the necessary deref to get to the actual value. The same happens
for indirect arguments that have the byval attribute. But the Rust ABI
does not use the byval attribute and so we need to manually add the
deref operation to the debuginfo.
@bors

bors commented Oct 18, 2017

Copy link
Copy Markdown
Collaborator

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing fc208bb to master...

@bors
bors merged commit 6bfecd4 into rust-lang:masterOct 18, 2017
@cuvipercuviper mentioned this pull request Nov 8, 2017
Closed
@dotdashdotdash mentioned this pull request Dec 19, 2017
Closed
@dotdash
dotdash deleted the dbg_bloat branch January 31, 2018 11:34
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@eddybeddybeddyb left review comments
@arielb1arielb1arielb1 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@michaelwoeristermichaelwoerister

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

11 participants

@dotdash@rust-highfive@eddyb@michaelwoerister@bors@TimNN@arielb1@aidanhs@alexcrichton@kennytm@nikomatsakis