Skip to content

llvm: change data layout bug to an error and make it trigger more - #120062

Merged
bors merged 1 commit into
rust-lang:masterfrom
davidtwco:llvm-data-layout-check
Jan 27, 2024
Merged

llvm: change data layout bug to an error and make it trigger more#120062
bors merged 1 commit into
rust-lang:masterfrom
davidtwco:llvm-data-layout-check

Conversation

@davidtwco

Copy link
Copy Markdown
Member

Fixes #33446.

Don't skip the inconsistent data layout check for custom LLVMs or non-built-in targets.

With #118708, all targets will have a simple test that would trigger this error if LLVM's data layouts do change - so data layouts would be corrected during the LLVM upgrade. Therefore, with builtin targets, this error won't happen with our LLVM because each target will have been confirmed to work. With non-builtin targets, this error is probably useful to have because you can change the data layout in your target and if it is wrong then that could lead to bugs.

When using a custom LLVM, the same justification makes sense for non-builtin targets as with our LLVM, the user can update their target to match their LLVM and that's probably a good thing to do. However, with a custom LLVM, the user cannot change the builtin target data layouts if they don't match - though given that the compiler's data layout is used for layout computation and a bunch of other things - you could get some bugs because of the mismatch and probably want to know about that. I'm not sure if this is something that people do and is okay, but I doubt it?

CFG_LLVM_ROOT was also always set during local development with download-ci-llvm so this bug would never trigger locally.

In #33446, two points are raised:

  • In the issue itself, changing this from a bug! to a proper error is what is suggested, by using isCompatibleDataLayout from LLVM, but that function still just does the same thing that we do and check for equality, so I've avoided the additional code necessary to do that FFI call.
  • @Mark-Simulacrum suggests a different check is necessary to maintain backwards compatibility with old LLVM versions. I don't know how often this comes up, but we can do that with some simple string manipulation + LLVM version checks as happens already for LLVM 17 just above this diff.

@rustbot

rustbot commented Jan 17, 2024

Copy link
Copy Markdown
Collaborator

r? @wesleywiser

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

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 17, 2024
@rust-log-analyzer

This comment has been minimized.

Comment thread tests/ui/codegen/mismatched-data-layouts.stderr Outdated
Don't skip the inconsistent data layout check for custom LLVMs.

With rust-lang#118708, all targets will have a simple test that would trigger this
check if LLVM's data layouts do change - so data layouts would be
corrected during the LLVM upgrade. Therefore, with builtin targets, this
check won't trigger with our LLVM because each target will have been
confirmed to work. With non-builtin targets, this check is probably
useful to have because you can change the data layout in your target and
if its wrong then that could lead to bugs.

When using a custom LLVM, the same justification makes sense for
non-builtin targets as with our LLVM, the user can update their target to
match their LLVM and that's probably a good thing to do. However, with
a custom LLVM, the user cannot change the builtin target data layouts if
they don't match - though given that the compiler's data layout is used
for layout computation and a bunch of other things - you could get some
bugs because of the mismatch and probably want to know about that.

`CFG_LLVM_ROOT` was also always set during local development with
`download-ci-llvm` so this bug would never trigger locally.

Signed-off-by: David Wood <david@davidtw.co>
@davidtwco
davidtwco force-pushed the llvm-data-layout-check branch from 8801304 to 46652dd Compare January 18, 2024 10:46
Comment thread compiler/rustc_codegen_llvm/src/context.rs
Comment thread tests/ui/codegen/mismatched-data-layouts.stderr
@wesleywiser

ghost commented Jan 23, 2024

Copy link
Copy Markdown
Member

Thanks @davidtwco!

@bors r+

@bors

ghost commented Jan 23, 2024

Copy link
Copy Markdown
Collaborator

📌 Commit 46652dd has been approved by wesleywiser

It is now in the queue for this repository.

@bors bors 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 Jan 23, 2024
@bors

ghost commented Jan 27, 2024

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 46652dd with merge 8af70c7...

@bors

ghost commented Jan 27, 2024

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 8af70c7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 27, 2024
@bors
bors merged commit 8af70c7 into rust-lang:master Jan 27, 2024
@rustbot rustbot added this to the 1.77.0 milestone Jan 27, 2024
@rust-timer

ghost commented Jan 27, 2024

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (8af70c7): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.6% [2.2%, 3.0%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.6% [2.2%, 3.0%] 2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.4% [2.4%, 2.4%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 660.284s -> 662.371s (0.32%)
Artifact size: 308.12 MiB -> 308.10 MiB (-0.00%)

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

Labels

A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Prevent "Target-incompatible DataLayout" LLVM asserts.

7 participants