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

rustc: Work around `DICompileUnit` bugs in LLVM #46772

Merged
merged 1 commit into from Dec 21, 2017

Conversation

Projects
None yet
5 participants
@alexcrichton
Member

alexcrichton commented Dec 16, 2017

This commit implements a workaround for #46346 which basically just
avoids triggering the situation that LLVM's bug
https://bugs.llvm.org/show_bug.cgi?id=35562 arises. More details can be
found in the code itself but this commit is also intended to ...

Closes #46346

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton
Member

alexcrichton commented Dec 16, 2017

@michaelwoerister

This comment has been minimized.

Show comment
Hide comment
@michaelwoerister

michaelwoerister Dec 16, 2017

Contributor

Thanks, Alex!

Do we have some kind of guarantee that the first DICompileUnit is the main one for the given module? Could you please add a check for that?

I'm a bit wary of the consequences of this patch. LLVM does not crash any more but how much will it break line information? I'm OK with merging for now but before we make ThinLTO the default for release builds we should verify that backtraces and basic profiling still work.

Also, we should really push for this being properly fixed in LLVM.

Contributor

michaelwoerister commented Dec 16, 2017

Thanks, Alex!

Do we have some kind of guarantee that the first DICompileUnit is the main one for the given module? Could you please add a check for that?

I'm a bit wary of the consequences of this patch. LLVM does not crash any more but how much will it break line information? I'm OK with merging for now but before we make ThinLTO the default for release builds we should verify that backtraces and basic profiling still work.

Also, we should really push for this being properly fixed in LLVM.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Dec 17, 2017

Member

Heh I was hoping you'd know more than I on those topics :)

I'm not actually sure at all what DICompileUnit does in terms of the debugging experience. Currently it looks like we generate things like:

!16 = !DIFile(filename: "foo.rs/@/foo0", directory: "/Users/acrichton/code/rust2")

So given that the file doesn't exist I'd imagine that this wouldn't impact the experience too much. That being said though were there particular things you're worried about? The backtraces do indeed seem to work on OSX (don't have access to Linux right now) but this would also be pretty easy to verify after-the-fact of landing.

Member

alexcrichton commented Dec 17, 2017

Heh I was hoping you'd know more than I on those topics :)

I'm not actually sure at all what DICompileUnit does in terms of the debugging experience. Currently it looks like we generate things like:

!16 = !DIFile(filename: "foo.rs/@/foo0", directory: "/Users/acrichton/code/rust2")

So given that the file doesn't exist I'd imagine that this wouldn't impact the experience too much. That being said though were there particular things you're worried about? The backtraces do indeed seem to work on OSX (don't have access to Linux right now) but this would also be pretty easy to verify after-the-fact of landing.

@michaelwoerister

This comment has been minimized.

Show comment
Hide comment
@michaelwoerister

michaelwoerister Dec 18, 2017

Contributor

The DICompileUnit sets the base path for things in that compile unit. So if there is a DIFile with a relative path instead of an absolute one then it is interpreted as being relative to the DW_AT_comp_dir that the compile unit specifies.

As far as paths go, we might be in the clear though, since all compile units from the same compilation session with have the same directory there.

Contributor

michaelwoerister commented Dec 18, 2017

The DICompileUnit sets the base path for things in that compile unit. So if there is a DIFile with a relative path instead of an absolute one then it is interpreted as being relative to the DW_AT_comp_dir that the compile unit specifies.

As far as paths go, we might be in the clear though, since all compile units from the same compilation session with have the same directory there.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Dec 18, 2017

Member

Ah excellent points @michaelwoerister! Useful to know for sure that DW_AT_comp_dir is set from this.

I think in that case this may end up working out well? In debug mode this'll never happen because we only generate one codegen unit and ThinLTO isn't used, and then in optimized mode debuginfo is sort of sketchy anyway, but it'd only worsen the experience with debugging the inlined functions from other libraries I think? (which may not have been that great to begin with...)

In that sense perhaps we should merge and see what happens? It's surely easy to turn off quickly :)

Member

alexcrichton commented Dec 18, 2017

Ah excellent points @michaelwoerister! Useful to know for sure that DW_AT_comp_dir is set from this.

I think in that case this may end up working out well? In debug mode this'll never happen because we only generate one codegen unit and ThinLTO isn't used, and then in optimized mode debuginfo is sort of sketchy anyway, but it'd only worsen the experience with debugging the inlined functions from other libraries I think? (which may not have been that great to begin with...)

In that sense perhaps we should merge and see what happens? It's surely easy to turn off quickly :)

@michaelwoerister

This comment has been minimized.

Show comment
Hide comment
@michaelwoerister

michaelwoerister Dec 18, 2017

Contributor

In that sense perhaps we should merge and see what happens?

Yes. If you could find a way to make sure that we keep the "main" DICompileUnit and only throw away the inlined ones then I agree that we should merge and see what happens.

Contributor

michaelwoerister commented Dec 18, 2017

In that sense perhaps we should merge and see what happens?

Yes. If you could find a way to make sure that we keep the "main" DICompileUnit and only throw away the inlined ones then I agree that we should merge and see what happens.

rustc: Work around `DICompileUnit` bugs in LLVM
This commit implements a workaround for #46346 which basically just
avoids triggering the situation that LLVM's bug
https://bugs.llvm.org/show_bug.cgi?id=35562 arises. More details can be
found in the code itself but this commit is also intended to ...

Closes #46346
@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Dec 18, 2017

Member

@bors: r=michaelwoerister

Member

alexcrichton commented Dec 18, 2017

@bors: r=michaelwoerister

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 18, 2017

Contributor

📌 Commit e0ab5d5 has been approved by michaelwoerister

Contributor

bors commented Dec 18, 2017

📌 Commit e0ab5d5 has been approved by michaelwoerister

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 21, 2017

Contributor

⌛️ Testing commit e0ab5d5 with merge de38f49...

Contributor

bors commented Dec 21, 2017

⌛️ Testing commit e0ab5d5 with merge de38f49...

bors added a commit that referenced this pull request Dec 21, 2017

Auto merge of #46772 - alexcrichton:thinlto-passes, r=michaelwoerister
rustc: Work around `DICompileUnit` bugs in LLVM

This commit implements a workaround for #46346 which basically just
avoids triggering the situation that LLVM's bug
https://bugs.llvm.org/show_bug.cgi?id=35562 arises. More details can be
found in the code itself but this commit is also intended to ...

Closes #46346
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 21, 2017

Contributor

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

Contributor

bors commented Dec 21, 2017

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

@bors bors merged commit e0ab5d5 into rust-lang:master Dec 21, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Dec 21, 2017

Member

(this might also be a fix for #46505, as noted by @kennytm )

Member

pnkfelix commented Dec 21, 2017

(this might also be a fix for #46505, as noted by @kennytm )

@alexcrichton alexcrichton deleted the alexcrichton:thinlto-passes branch Jan 23, 2018

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