Skip to content
This repository has been archived by the owner on Feb 5, 2019. It is now read-only.

Fix assertion in FunctionComparator::cmpInlineAsm #111

Merged
merged 1 commit into from
May 10, 2018

Conversation

nox
Copy link

@nox nox commented May 5, 2018

@nox
Copy link
Author

nox commented May 5, 2018

Cc @rkruppe @eddyb @nikic

@hanna-kruppe
Copy link

I'd prefer the commit message (and possibly even a comment in the code) to mention the LLVM bug and that this is a temporary workaround until there is an upstream fix. It's possible that upstream will do the same thing, but if not, we should backport the upstream patch eventually.

@nox
Copy link
Author

nox commented May 5, 2018

The LLVM bug is mentioned in the commit message.

@hanna-kruppe
Copy link

Oh, sorry for the noise.

@nox
Copy link
Author

nox commented May 5, 2018

@eddyb told me to bug you, so here I am @alexcrichton.

@alexcrichton
Copy link
Member

Thanks! We tend to not accept patches though that aren't in upstream LLVM, so can this be submitted upstream first?

@nox
Copy link
Author

nox commented May 5, 2018 via email

@alexcrichton
Copy link
Member

It's effectively our policy at this point that changes like this aren't accepted unless they're in upstream LLVM first, so unless there's extenuating circumstances this should be reviewed by LLVM authors first.

@nikic
Copy link

nikic commented May 5, 2018

I've submitted this change and a test based on @rkruppe's reduced IR for review upstream at https://reviews.llvm.org/D46495.

@nox
Copy link
Author

nox commented May 9, 2018

@alexcrichton It seems to me that @nikic's patch was accepted upstream, can we backport it here now?

This revision is now accepted and ready to land.

@alexcrichton
Copy link
Member

Yes once there's an LLVM commit in the git mirror to cherry-pick then it should be cherry-picked.

@nikic
Copy link

nikic commented May 10, 2018

Change has landed upstream via llvm-mirror/llvm@0ccb7f4, so can be cherry-picked now :)

Fixes bug https://bugs.llvm.org/show_bug.cgi?id=37339.

InlineAsm is only uniqued if the FunctionTypes are exactly the
same, while cmpTypes() for example considers all pointer types
in the default address space to be the same. For this reason
the end of cmpInlineAsm() can be reached.

This patch replaces the unreachable assertion with a check that
the function types are not identical.

Differential Revision: https://reviews.llvm.org/D46495

Reviewers: jfb

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@331990 91177308-0d34-0410-b5e6-96231b3b80d8
@nox nox changed the title Relax the assertion in cmpInlineAsm Fix assertion in FunctionComparator::cmpInlineAsm May 10, 2018
@nox
Copy link
Author

nox commented May 10, 2018

I cherry-picked it in this very PR. LGTM?

@alexcrichton alexcrichton merged commit da0cb60 into rust-lang:rust-llvm-release-6-0-0 May 10, 2018
@nox nox deleted the mergefunc branch May 10, 2018 17:14
nox added a commit to nox/rust that referenced this pull request May 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants