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

rustc silently ignores a class of errors in flexible target json files #24384

Closed
ghost opened this issue Apr 13, 2015 · 5 comments
Closed

rustc silently ignores a class of errors in flexible target json files #24384

ghost opened this issue Apr 13, 2015 · 5 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-target-specs Area: Compile-target specifications C-bug Category: This is a bug.

Comments

@ghost
Copy link

ghost commented Apr 13, 2015

Currently rustc will allow for extra fields to be added to flexible target json without notifying the user, even with --verbose. For example, the following is accepted:

{
    "THIS-IS-INCORRECT": "but rustc will silently accept it anyway",
    "data-layout": "e-p:32:32-f64:32:64-i64:32:64-f80:32:32-n8:16:32",
    "llvm-target": "i686-unknown-linux-gnu",
    "target-endian": "little",
    "target-pointer-width": "32",
    "arch": "x86",
    "os": "linux",
    "morestack": "false",
    "pre-link-args": "-nostdlib",
    "dynamic_linking": false
}
  • dynamic_linking has a underscore separator instead of a hyphen.
  • morestack is of type bool, but a string was provided
  • pre-link-args takes an array of strings, but a string was provided

The last three entries have errors that could be easy to miss. This is worrying, because for all the optional fields, simple errors such as misspellings will cause those fields to be discarded and the default values to be used instead. To the programmer it could seem as if the configuration is in place when in reality the compiler simply went with the default value.

I had one of these happen to me earlier and I almost missed it completely. I think that this should at the very least trigger a warning.

@steveklabnik steveklabnik added I-wrong A-diagnostics Area: Messages for errors, warnings, and lints and removed I-wrong labels Apr 16, 2015
@steveklabnik
Copy link
Member

This isn't strictly diagnostics, but close enough.

@emberian
Copy link
Member

Yeah, this has annoyed me recently too, when I originally did this I decided not to validate the precense of extra fields for compatability with future fields in other rustc versions (As otherwise it's not possible to have a new target spec work on an old rustc at all). I'm pretty sure that we should NOTABUG that, but do stricter validation for types.

@steveklabnik
Copy link
Member

Triage: I don't believe that anything has changed here, but I can't check exactly right now.

@Mark-Simulacrum
Copy link
Member

This seems like we'd want to do stricter validation of types. I'm somewhat surprised we don't today, but it's possible this could be fixed by moving to Serde for deserialization (even if just into serde_json::Value) since it does far more strict validations.

@Mark-Simulacrum Mark-Simulacrum added the A-target-specs Area: Compile-target specifications label Jun 22, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-bug Category: This is a bug. label Jul 22, 2017
@steveklabnik
Copy link
Member

So, I thought that there was a PR to fix this, but I cannot find it. Regardless, when trying to build a project using the above target spec:

error: failed to run `rustc` to learn about target-specific information

Caused by:
  process didn't exit successfully: `rustc - --crate-name ___ --print=file-names --target \\?\C:\Users\steve\src\intermezzos\kernel\intermezzos.json -
-crate-type bin --crate-type rlib --crate-type dylib --crate-type cdylib --crate-type staticlib --crate-type proc-macro` (exit code: 1)
--- stderr
error: Error loading target specification: Field target-c-int-width in target specification is required
  |
  = help: Use `--print target-list` for a list of built-in targets

fixing some of it up:

error: Error loading target specification: pre-link-args: expected a JSON object with fields per linker-flavor.

so, it does seem like this has been fixed. Closing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-target-specs Area: Compile-target specifications C-bug Category: This is a bug.
Projects
None yet
Development

No branches or pull requests

3 participants