tests: add sanity-check assembly test for every target #118708

davidtwco · 2023-12-07T14:18:28Z

Fixes #119910.

Adds a basic assembly test checking that each target can produce assembly and update the target tier policy to require this.

cc rust-lang/compiler-team#655
r? @wesleywiser

rustbot · 2023-12-07T14:18:31Z

Could not assign reviewer from: wesleywiser.
User(s) wesleywiser are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

rustbot · 2023-12-07T14:18:37Z

r? @Mark-Simulacrum

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

tests/assembly/targets.rs

Mark-Simulacrum · 2023-12-09T14:57:37Z

r=me

davidtwco · 2023-12-11T15:45:39Z

@bors r=Mark-Simulacrum

Rebased after some new targets were added but otherwise no meaningful changes.

bors · 2023-12-11T15:45:41Z

📌 Commit 6c993e4 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

davidtwco · 2023-12-11T15:45:53Z

@bors rollup

…t, r=Mark-Simulacrum tests: add sanity-check assembly test for every target Adds a basic assembly test checking that each target can produce assembly and update the target tier policy to require this. cc rust-lang/compiler-team#655 r? `@wesleywiser`

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#118647 (dump bootstrap shims) - rust-lang#118708 (tests: add sanity-check assembly test for every target) - rust-lang#118797 (End locals' live range before suspending coroutine) - rust-lang#118818 (llvm-wrapper: adapt for LLVM API change) - rust-lang#118826 (Edit target doc template to remove email) - rust-lang#118827 (Update table for linker-plugin-lto docs) r? `@ghost` `@rustbot` modify labels: rollup

…t, r=Mark-Simulacrum tests: add sanity-check assembly test for every target Adds a basic assembly test checking that each target can produce assembly and update the target tier policy to require this. cc rust-lang/compiler-team#655 r? ``@wesleywiser``

…t, r=Mark-Simulacrum tests: add sanity-check assembly test for every target Adds a basic assembly test checking that each target can produce assembly and update the target tier policy to require this. cc rust-lang/compiler-team#655 r? ```@wesleywiser```

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#118647 (dump bootstrap shims) - rust-lang#118708 (tests: add sanity-check assembly test for every target) - rust-lang#118818 (llvm-wrapper: adapt for LLVM API change) - rust-lang#118826 (Edit target doc template to remove email) - rust-lang#118827 (Update table for linker-plugin-lto docs) - rust-lang#118835 (Fix again `rustc_codegen_gcc` tests) r? `@ghost` `@rustbot` modify labels: rollup

matthiaskrgr · 2023-12-11T19:45:59Z

@bors r- rollup=iffy
#118836 (comment)

davidtwco · 2024-01-11T11:28:43Z

There have been no meaningful changes since last approval, just updating the test being added with targets that have been added recently.

@bors=wesleywiser

bors · 2024-01-15T21:09:06Z

💔 Test failed - checks-actions

Adds a basic assembly test checking that each target can produce assembly and update the target tier policy to require this. Signed-off-by: David Wood <[email protected]>

In LLVM 17, PowerPC targets started including function pointer alignments in data layouts, and in Rust's update to that version (rust-lang#114048), we added the function pointer alignments. `powerpc64-unknown-linux-musl` had `Fi64` set but this seems incorrect, and the code in LLVM would always have computed `Fn32` because it is a MUSL target. Signed-off-by: David Wood <[email protected]>

davidtwco · 2024-01-17T10:49:30Z

So the error for incompatible data layouts doesn't happen locally because of..

rust/src/bootstrap/src/core/build_steps/compile.rs

Lines 1113 to 1115 in 16f4b02

    
           if let Some(s) = target_config.and_then(|c| c.llvm_config.as_ref()) { 
        
               cargo.env("CFG_LLVM_ROOT", s); 
        
           }

..which prevents the ICE...

rust/compiler/rustc_codegen_llvm/src/context.rs

Lines 159 to 177 in 16f4b02

    
           // Unfortunately LLVM target specs change over time, and right now we 
        
           // don't have proper support to work with any more than one 
        
           // `data_layout` than the one that is in the rust-lang/rust repo. If 
        
           // this compiler is configured against a custom LLVM, we may have a 
        
           // differing data layout, even though we should update our own to use 
        
           // that one. 
        
           // 
        
           // As an interim hack, if CFG_LLVM_ROOT is not an empty string then we 
        
           // disable this check entirely as we may be configured with something 
        
           // that has a different target layout. 
        
           // 
        
           // Unsure if this will actually cause breakage when rustc is configured 
        
           // as such. 
        
           // 
        
           // FIXME(#34960) 
        
           let cfg_llvm_root = option_env!("CFG_LLVM_ROOT").unwrap_or(""); 
        
           let custom_llvm_used = !cfg_llvm_root.trim().is_empty(); 
        
           if !custom_llvm_used && target_data_layout != llvm_data_layout {

cc #34960 #33446

davidtwco · 2024-01-17T14:35:33Z

@bors r=Mark-Simulacrum

bors · 2024-01-17T14:35:36Z

📌 Commit 12c19a2 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

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 <[email protected]>

bors · 2024-01-17T16:18:32Z

⌛ Testing commit 12c19a2 with merge 6ae4cfb...

bors · 2024-01-17T18:44:19Z

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 6ae4cfb to master...

rust-timer · 2024-01-17T20:39:51Z

Finished benchmarking commit (6ae4cfb): 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)	-	-	0
Regressions ❌ (secondary)	1.1%	[1.1%, 1.1%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-5.3%	[-8.2%, -3.2%]	17
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 664.091s -> 663.454s (-0.10%)
Artifact size: 308.23 MiB -> 308.30 MiB (0.02%)

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 <[email protected]>

rust-lang/rust#118708

…r=wesleywiser llvm: change data layout bug to an error and make it trigger more Fixes rust-lang#33446. Don't skip the inconsistent data layout check for custom LLVMs or non-built-in targets. With rust-lang#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 rust-lang#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.

…r=wesleywiser llvm: change data layout bug to an error and make it trigger more Fixes rust-lang#33446. Don't skip the inconsistent data layout check for custom LLVMs or non-built-in targets. With rust-lang#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 rust-lang#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.

…r=wesleywiser llvm: change data layout bug to an error and make it trigger more Fixes rust-lang#33446. Don't skip the inconsistent data layout check for custom LLVMs or non-built-in targets. With rust-lang#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 rust-lang#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.

…r=wesleywiser llvm: change data layout bug to an error and make it trigger more Fixes rust-lang#33446. Don't skip the inconsistent data layout check for custom LLVMs or non-built-in targets. With rust-lang#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 rust-lang#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.

…wesleywiser llvm: change data layout bug to an error and make it trigger more Fixes rust-lang#33446. Don't skip the inconsistent data layout check for custom LLVMs or non-built-in targets. With rust-lang#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 rust-lang#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 assigned Mark-Simulacrum Dec 7, 2023

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) labels Dec 7, 2023

davidtwco mentioned this pull request Dec 7, 2023

target tier policy: require ability to produce object code #115217

Closed

This comment was marked as resolved.

Sign in to view

davidtwco force-pushed the target-tier-assembly-test branch from 51aa2e7 to 605ac00 Compare December 8, 2023 14:00

Mark-Simulacrum reviewed Dec 9, 2023

View reviewed changes

tests/assembly/targets.rs Outdated Show resolved Hide resolved

davidtwco force-pushed the target-tier-assembly-test branch from 605ac00 to 6c993e4 Compare December 11, 2023 13:47

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 Dec 11, 2023

matthiaskrgr mentioned this pull request Dec 11, 2023

Rollup of 6 pull requests #118831

Closed

matthiaskrgr mentioned this pull request Dec 11, 2023

Rollup of 5 pull requests #118834

Closed

matthiaskrgr mentioned this pull request Dec 11, 2023

Rollup of 6 pull requests #118836

Closed

bors 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 Dec 11, 2023

davidtwco force-pushed the target-tier-assembly-test branch from 6c993e4 to d35a3ce Compare January 11, 2024 10:41

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 15, 2024

tests: add sanity-check assembly test for every target

a87034c

Adds a basic assembly test checking that each target can produce assembly and update the target tier policy to require this. Signed-off-by: David Wood <[email protected]>

davidtwco mentioned this pull request Jan 17, 2024

powerpc64-unknown-linux-musl target data-layout does not match LLVM, making the target useless #119910

Closed

davidtwco force-pushed the target-tier-assembly-test branch from 4a8e90b to 12c19a2 Compare January 17, 2024 10:43

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 17, 2024

davidtwco mentioned this pull request Jan 17, 2024

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

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 17, 2024

bors merged commit 6ae4cfb into rust-lang:master Jan 17, 2024
12 checks passed

rustbot added this to the 1.77.0 milestone Jan 17, 2024

davidtwco deleted the target-tier-assembly-test branch January 18, 2024 09:35

Dajamante added a commit to ferrocene/ferrocene that referenced this pull request Jan 19, 2024

assembly sanity check for ferrocene targets

209f9a4

rust-lang/rust#118708

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: add sanity-check assembly test for every target #118708

tests: add sanity-check assembly test for every target #118708

davidtwco commented Dec 7, 2023 •

edited

Loading

rustbot commented Dec 7, 2023

rustbot commented Dec 7, 2023

This comment was marked as resolved.

Mark-Simulacrum commented Dec 9, 2023

davidtwco commented Dec 11, 2023

bors commented Dec 11, 2023

davidtwco commented Dec 11, 2023

matthiaskrgr commented Dec 11, 2023

davidtwco commented Jan 11, 2024

This comment has been minimized.

bors commented Jan 15, 2024

davidtwco commented Jan 17, 2024

davidtwco commented Jan 17, 2024

bors commented Jan 17, 2024

bors commented Jan 17, 2024

bors commented Jan 17, 2024

rust-timer commented Jan 17, 2024

tests: add sanity-check assembly test for every target #118708

tests: add sanity-check assembly test for every target #118708

Conversation

davidtwco commented Dec 7, 2023 • edited Loading

rustbot commented Dec 7, 2023

rustbot commented Dec 7, 2023

This comment was marked as resolved.

Mark-Simulacrum commented Dec 9, 2023

davidtwco commented Dec 11, 2023

bors commented Dec 11, 2023

davidtwco commented Dec 11, 2023

matthiaskrgr commented Dec 11, 2023

davidtwco commented Jan 11, 2024

This comment has been minimized.

bors commented Jan 15, 2024

davidtwco commented Jan 17, 2024

davidtwco commented Jan 17, 2024

bors commented Jan 17, 2024

bors commented Jan 17, 2024

bors commented Jan 17, 2024

rust-timer commented Jan 17, 2024

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

davidtwco commented Dec 7, 2023 •

edited

Loading