Skip to content

Fixes for LLVM change 0f45c16f2caa7c035e5c3edd40af9e0d51ad6ba7 - #88289

Merged
bors merged 1 commit into
rust-lang:masterfrom
durin42:llvm-14-attrs
Aug 27, 2021
Merged

Fixes for LLVM change 0f45c16f2caa7c035e5c3edd40af9e0d51ad6ba7#88289
bors merged 1 commit into
rust-lang:masterfrom
durin42:llvm-14-attrs

Conversation

@durin42

@durin42durin42 commented Aug 24, 2021

Copy link
Copy Markdown
Contributor

More details in the individual commit messages, but the summary is: LLVM deleted an unused-to-them method that we used, we worked around it to avoid annoying cleanup/restructuring in the Rust-side code.

@rust-highfive

rust-highfive commented Aug 24, 2021

Copy link
Copy Markdown
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jackh726 (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 24, 2021
nikic
nikic reviewed Aug 24, 2021
View reviewed changes
Comment threadcompiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp Outdated
@nikic

nikic commented Aug 24, 2021

Copy link
Copy Markdown
Contributor

I think the right way now would be use lower-level APIs, something like this:

AttributeList AL = F->getAttributes();
AL = AL.addAttributes(F->getContext(), Index, B);
F->setAttributes(AL);

@aeubanks Does that sound right?

@aeubanks

aeubanks commented Aug 24, 2021

Copy link
Copy Markdown
Contributor

These all look like single attributes, is an AttrBuilder necessary? Can you just directly create and add an Attribute?

@nikic

nikic commented Aug 24, 2021

Copy link
Copy Markdown
Contributor

@aeubanks Good point, I missed that. Yes, it should be possible to use APIs like Attribute::getWithDereferenceableBytes() to get the Attribute and add that.

@inquisitivecrystalinquisitivecrystal added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 24, 2021
@durin42

durin42 commented Aug 25, 2021

Copy link
Copy Markdown
ContributorAuthor

Alright, I believe I've cleaned this up to use all the right APIs. I'm a little dubious of the one static_cast<uint64_t> I had to use, and there's only one callsite in the Rust code, so maybe that code should be doing as u64 instead of as u32? Let me know if you want me to fix that too.

@jackh726

jackh726 commented Aug 25, 2021

Copy link
Copy Markdown
Member

r? @nikic

@rust-highfiverust-highfive assigned nikic and unassigned jackh726Aug 25, 2021
@aeubanks

aeubanks commented Aug 25, 2021

Copy link
Copy Markdown
Contributor

I don't think you need the #ifdefs, you should be able to use the newly suggested method even with older versions of LLVM. It's cleaner anyway.

nikic
nikic reviewed Aug 25, 2021
View reviewed changes
Comment threadcompiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp Outdated
Comment threadcompiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp Outdated
@durin42

durin42 commented Aug 25, 2021

Copy link
Copy Markdown
ContributorAuthor

Dropped the ifdefs, verified everything works on LLVM HEAD. I assume the bots will cover the older versions of LLVM in case I messed something up.

nikic
nikic approved these changes Aug 25, 2021
View reviewed changes

@nikicnikic left a comment

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.

Looks good! Could you please squash the commits?

@durin42

durin42 commented Aug 25, 2021

Copy link
Copy Markdown
ContributorAuthor

Squashed. Thanks!

@nikic

nikic commented Aug 25, 2021

Copy link
Copy Markdown
Contributor

@bors r+ rollup

@bors

bors commented Aug 25, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit f751e2554dffe583d1c525ca5a3a28bf81a65d67 has been approved by nikic

@borsbors 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 Aug 25, 2021
aeubanks
aeubanks reviewed Aug 25, 2021
View reviewed changes
Comment threadcompiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp Outdated
@nikic

nikic commented Aug 26, 2021

Copy link
Copy Markdown
Contributor

@bors r- per Arthur's comment above

@borsbors 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 Aug 26, 2021
@durin42
RustWrapper: adapt to LLVM change0f45c16
027db5d
The above-mentioned commit (part of the LLVM 14 development cycle)
removes a method that rustc uses somewhat extensively. We mostly switch
to lower-level methods that exist in all versions of LLVM we use, so no
new ifdef logic is required in most cases.
@durin42

durin42 commented Aug 26, 2021

Copy link
Copy Markdown
ContributorAuthor

PTAL - this should be ready

@nikic

nikic commented Aug 26, 2021

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Aug 26, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit 027db5d has been approved by nikic

@borsbors 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 Aug 26, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Aug 26, 2021
@Dylan-DPC
Rollup merge ofrust-lang#88289- durin42:llvm-14-attrs, r=nikic
bc669ac
Fixes for LLVM change 0f45c16
More details in the individual commit messages, but the summary is: LLVM deleted an unused-to-them method that we used, we worked around it to avoid annoying cleanup/restructuring in the Rust-side code.
@Dylan-DPC-zzDylan-DPC-zz mentioned this pull request Aug 26, 2021
Closed
@ManishearthManishearth mentioned this pull request Aug 26, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 27, 2021
@bors
Auto merge ofrust-lang#88371- Manishearth:rollup-pkkjsme, r=Manishe…
d5cd320
…arth
Rollup of 11 pull requests
Successful merges:
- rust-lang#87832 (Fix debugger stepping behavior with `match` expressions)
- rust-lang#88123 (Make spans for tuple patterns in E0023 more precise)
- rust-lang#88215 (Reland rust-lang#83738: "rustdoc: Don't load all extern crates unconditionally")
- rust-lang#88216 (Don't stabilize creation of TryReserveError instances)
- rust-lang#88270 (Handle type ascription type ops in NLL HRTB diagnostics)
- rust-lang#88289 (Fixes for LLVM change 0f45c16)
- rust-lang#88320 (type_implements_trait consider obligation failure on overflow)
- rust-lang#88332 (Add argument types tait tests)
- rust-lang#88340 (Add `c_size_t` and `c_ssize_t` to `std::os::raw`.)
- rust-lang#88346 (Revert "Add type of a let tait test impl trait straight in let")
- rust-lang#88348 (Add field types tait tests)
Failed merges:
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit dfca7b3 into rust-lang:masterAug 27, 2021
@rustbotrustbot added this to the 1.56.0 milestone Aug 27, 2021
@durin42
durin42 deleted the llvm-14-attrs branch September 7, 2021 18:52
@nikicnikic mentioned this pull request Sep 7, 2021
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nikicnikicnikic approved these changes
+1 more reviewer
@aeubanksaeubanksaeubanks left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@nikicnikic

Labels

A-LLVMArea: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.56.0

Development

Successfully merging this pull request may close these issues.

8 participants

@durin42@rust-highfive@nikic@aeubanks@jackh726@bors@inquisitivecrystal@rustbot