Skip to content
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

Prevent "Target-incompatible DataLayout" LLVM asserts. #33446

Closed
eddyb opened this issue May 5, 2016 · 9 comments · Fixed by #120062
Closed

Prevent "Target-incompatible DataLayout" LLVM asserts. #33446

eddyb opened this issue May 5, 2016 · 9 comments · Fixed by #120062
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@eddyb
Copy link
Member

eddyb commented May 5, 2016

Example of such an assertion:
Assertion 'TM.isCompatibleDataLayout(getDataLayout()) && "Can't create a MachineFunction using a Module with a " "Target-incompatible DataLayout attached\n"' failed.

It appears that since LLVM 3.7, DataLayout is checked for target compatibility, and as joker-eph on LLVM's IRC channel on OFTC explains:

technically a Target can implement a "compatibility check" if it is willing to relax the constrain that the DL matches, but we haven't implemented anything like that that I know of. So the backend check if the Module data layout is the same as what the target wants.

The relevant method is bool TargetMachine::isCompatibleDataLayout(const DataLayout &Candidate).

joker-eph suggests warning and overriding an incompatible data layout with the target default.
However, I don't think we can do anything but error because we would've already used the (incompatible) data layout string for determining type layouts.

We can at least provide the target default data layout string for the user in that situation.
cc @alexcrichton

@nagisa
Copy link
Member

nagisa commented May 6, 2016

Perhaps we should also check that the target with the mismatching datalayout is not built-in and if it is, emit an ICE.

@eddyb
Copy link
Member Author

eddyb commented May 6, 2016

@nagisa There's already a check that the built-in datalayouts are identical to the target defaults.

@alexcrichton
Copy link
Member

If this is the case, should we just go back to picking it up from LLVM by default, but allowing a specification for a custom one?

@eddyb
Copy link
Member Author

eddyb commented May 6, 2016

@alexcrichton If we want to build a WASM backend, for example, how could that work without linking LLVM? Do we want to couple target specifications to LLVM in such a way that you need special ones for non-LLVM backends?

@alexcrichton
Copy link
Member

Hm perhaps, yeah. This is kinda just an area where custom target specs aren't exactly intended to be stabilized as how would a wasm backend read and use a custom target spec anyway?

@eddyb
Copy link
Member Author

eddyb commented May 6, 2016

@alexcrichton If you use the driver it's all automated.
In a way, it doesn't make sense to have target specs for a wasm backend, there's one wasm "target spec".

What do you think about having librustc_driver pass some librustc_trans validator/elaborator to librustc_back?
This would alleviate the need for several options which are currently mandatory, if "llvm-target" is present, as they can be extracted from that by querying LLVM itself.

I would keep all the information for the built-in targets there, just so we can get alerted if anything doesn't match the LLVM defaults for that specific triple, but target specs would have no guarantees of compatibility across LLVM versions.

To get back to wasm: it could have a target spec that doesn't contain "llvm-target", so that you can't actually use the LLVM backend with it, but it includes all information needed to otherwise run all analysis passes.

@alexcrichton
Copy link
Member

Sounds plausible to me!

@Mark-Simulacrum Mark-Simulacrum 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 Jun 23, 2017
@Mark-Simulacrum
Copy link
Member

From #34960:

Discovered in #34743, LLVM's data layouts are changing over time. If we assert that our own data layout agrees with LLVM's then if we change to a newer LLVM we break compatibility with older LLVMs. It looks like the change to the data layout was backwards compatible, I think, so we probably just need a different check here.

@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 25, 2017
@steveklabnik
Copy link
Member

Triage: not aware of any changes.

fmease added a commit to fmease/rust that referenced this issue Jan 23, 2024
…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.
fmease added a commit to fmease/rust that referenced this issue Jan 24, 2024
…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.
fmease added a commit to fmease/rust that referenced this issue Jan 24, 2024
…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.
fmease added a commit to fmease/rust that referenced this issue Jan 24, 2024
…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.
@bors bors closed this as completed in 8af70c7 Jan 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants