Skip to content

/rust

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

codegen-units setting in Cargo.toml causes LLVM error with nightly-i686-pc-windows-msvc #36309

Closed
Boscop opened this Issue Sep 6, 2016 · 10 comments

Comments

Assignees

@michaelwoerister michaelwoerister

Labels
A-codegen E-needstest O-windows P-medium T-compiler regression-from-stable-to-nightly
Projects
None yet
Milestone
No milestone
6 participants
@Boscop @brson @alexcrichton @michaelwoerister @pnkfelix @nrc
@Boscop
Copy link

Boscop commented Sep 6, 2016

Using the latest nightly rustc 1.13.0-nightly (cbe4de78e 2016-09-05) I'm getting this error on a project that uses euclid which uses xml-rs:
LLVM ERROR: assembler label 'L__ehtable$_ZN3xml6reader6parser10MarkupData13take_ encoding17hb15412b3bac19ec7E' can not be undefined error: Could not compile xml-rs.
This happens only when I have codegen-units = 4 in my Cargo.toml and only with the 32bit version of the toolchain, only with nightly-i686-pc-windows-msvc not with nightly-x86_64-pc-windows-msvc.
(I tested 4 configs: with/without codegen-units = 4 in Cargo.toml ⨯ 32/64 bit)

@brson brson added O-windows A-codegen regression-from-stable-to-nightly I-wrong T-compiler labels Sep 7, 2016

@brson

This comment has been minimized.

Sign in to view
Copy link
Contributor

brson commented Sep 7, 2016

cc @mw
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Sep 13, 2016

cc @michaelwoerister

@brson brson added the I-nominated label Sep 13, 2016

@brson

This comment has been minimized.

Sign in to view
Copy link
Contributor

brson commented Sep 13, 2016

Nominating for @rust-lang/compiler triage.
@michaelwoerister

This comment has been minimized.

Sign in to view
Copy link
Contributor

michaelwoerister commented Sep 13, 2016

I have a dim memory of fixing this exact same problem two months ago... 😕

@michaelwoerister michaelwoerister self-assigned this Sep 14, 2016

@michaelwoerister

This comment has been minimized.

Sign in to view
Copy link
Contributor

michaelwoerister commented Sep 14, 2016

I knew that I had seen this problem before somewhere: #33890 (comment)

@michaelwoerister michaelwoerister referenced this issue Sep 14, 2016

Closed

Don't generate code for unused #[inline] function #36280

@nrc nrc added P-medium and removed I-nominated labels Sep 15, 2016

@michaelwoerister michaelwoerister referenced this issue Sep 16, 2016

Merged

trans: Only instantiate #[inline] functions in codegen units referencing them #36524

bors added a commit that referenced this issue Sep 19, 2016

@bors
Auto merge of #36524 - michaelwoerister:trans-inline-only-on-demand, … 
…r=nikomatsakis

trans: Only instantiate #[inline] functions in codegen units referencing them

This PR changes how `#[inline]` functions are translated. Before, there was one "master instance" of the function with `external` linkage and a number of on-demand instances with `available_externally` linkage in each codegen unit that referenced the function. This had two downsides:

* Public functions marked with `#[inline]` would be present in machine code of libraries unnecessarily (see #36280 for an example)
* LLVM would crash on `i686-pc-windows-msvc` due to what I suspect to be a bug in LLVM's Win32 exception handling code, because it doesn't like `available_externally` there (#36309).

This PR changes the behavior, so that there is no master instance and only on-demand instances with `internal` linkage. The downside of this is potential code-bloat if LLVM does not completely inline away the `internal` instances because then there'd be N instances of the function instead of 1. However, this can only become a problem when using more than one codegen unit per crate.

cc @rust-lang/compiler
4f363c9

bors added a commit that referenced this issue Sep 21, 2016

@bors
Auto merge of #36524 - michaelwoerister:trans-inline-only-on-demand, … 
…r=nikomatsakis

trans: Only instantiate #[inline] functions in codegen units referencing them

This PR changes how `#[inline]` functions are translated. Before, there was one "master instance" of the function with `external` linkage and a number of on-demand instances with `available_externally` linkage in each codegen unit that referenced the function. This had two downsides:

* Public functions marked with `#[inline]` would be present in machine code of libraries unnecessarily (see #36280 for an example)
* LLVM would crash on `i686-pc-windows-msvc` due to what I suspect to be a bug in LLVM's Win32 exception handling code, because it doesn't like `available_externally` there (#36309).

This PR changes the behavior, so that there is no master instance and only on-demand instances with `internal` linkage. The downside of this is potential code-bloat if LLVM does not completely inline away the `internal` instances because then there'd be N instances of the function instead of 1. However, this can only become a problem when using more than one codegen unit per crate.

cc @rust-lang/compiler
5cc6c6b
@brson

This comment has been minimized.

Sign in to view
Copy link
Contributor

brson commented Sep 22, 2016

@michaelwoerister is this fixed?
@brson

This comment has been minimized.

Sign in to view
Copy link
Contributor

brson commented Sep 22, 2016

@Boscop maybe you can check if this is fixed?
@michaelwoerister

This comment has been minimized.

Sign in to view
Copy link
Contributor

michaelwoerister commented Sep 22, 2016

It should be fixed but I'm not sure the fix is in nightly yet.
@pnkfelix

This comment has been minimized.

Sign in to view
Copy link
Member

pnkfelix commented Sep 22, 2016

@michaelwoerister it should be in nightly, the most recent one is:

rustc 1.13.0-nightly (4f9812a 2016-09-21)

@pnkfelix pnkfelix added the E-needstest label Sep 29, 2016

@michaelwoerister

This comment has been minimized.

Sign in to view
Copy link
Contributor

michaelwoerister commented Oct 6, 2016

I spent some time trying to write a small test case without success. Since this is fixed for xml-rs which is cited by the original report, I'll close this.

@michaelwoerister michaelwoerister closed this Oct 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.