Skip to content

New error metadata system required. #26009

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

Closed
michaelsproul opened this issue Jun 4, 2015 · 3 comments · Fixed by #26452
Closed

New error metadata system required. #26009

michaelsproul opened this issue Jun 4, 2015 · 3 comments · Fixed by #26452
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one.

Comments

@michaelsproul
Copy link
Contributor

It'd be nice to hang on to the error index in the Rust 1.2 release... currently it's disabled due to the flakiness of the metadata collation system (see #25706).

Amongst the issues with the previous implementation:

  • Unsynchronised concurrent access of files, leading to the use of malformed JSON data.
  • No way to determine the freshness of metadata files.
  • Failure to write files (not thoroughly investigated). Probably related to stale files hanging around.

Before I go ahead and implement a crazy new system, I just want to check the sanity of my design:

  • Alter the __build_diagnostic_array! invocations so that the identifier used for diagnostics arrays is something unlikely to be used elsewhere. This step isn't strictly necessary, as the current name DIAGNOSTICS is already quite unique.
  • Register an internal lint (i.e. one that only runs against the compiler's source) that extracts error metadata from every item with the unique name used by __build_diagnostic_array!. Here we run into the same potential concurrency problem as before, because crates can be compiled in parallel. As long as the lint doesn't attempt to check uniqueness by reading files for other crates, the parallel writes to separate per-crate files should be ok. E.g. when running the lint against librustc_typeck, dump metadata to tmp/extended-errors/librustc_typeck.json. What worries me about this step is that it is essentially the same as dumping metadata from the __build_diagnostic_array! without uniqueness-checks, which we tried unsuccessfully (that's the unknown error).
  • In the error-index-generator - which runs once all crates have been compiled - we can check uniqueness.

I still feel like this is very similar to the previous, failing system, and would like to alter it more drastically if anyone has any ideas...

cc @pnkfelix @alexcrichton @Manishearth

@alexcrichton
Copy link
Member

As long as the lint doesn't attempt to check uniqueness by reading files for other crates, the parallel writes to separate per-crate files should be ok.

I do think that you can compile librustc_typeck in parallel with itself as well, however, for example with cross-compiled builds. I think that this worry could be alleviated by scoping the output to a target-specific location, however.

As long as the lint doesn't attempt to check uniqueness by reading files for other crates, the parallel writes to separate per-crate files should be ok.

I'd recommend putting the uniqueness part in the make tidy step personally, seems like a good scriptable task!

@michaelsproul
Copy link
Contributor Author

I do think that you can compile librustc_typeck in parallel with itself as well, however, for example with cross-compiled builds. I think that this worry could be alleviated by scoping the output to a target-specific location, however.

Good idea!

I'd recommend putting the uniqueness part in the make tidy step personally, seems like a good scriptable task!

Yep, we'll just have to make sure it doesn't fail if the JSON files aren't there, like whenmake tidy runs before the compiler has been built. Our current infrastructure also doesn't seem to contain any rules to ensure that tidy is run last when running check, although it seems it happens this way because of the dependency ordering - check: ... tidy.

@steveklabnik steveklabnik added A-build C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Jun 8, 2015
@steveklabnik
Copy link
Member

These labels aren't perfect, exactly.

Anyway, yes, it would be really nice not to lose this. I've heard so many people describe this as an important thing to them.

Manishearth added a commit to Manishearth/rust that referenced this issue Jun 20, 2015
…pnkfelix

As per rust-lang#26009 this PR implements a new collation system for extended-error metadata. I've tried to keep it as simple as possible for now, so there's no uniqueness checking and minimal modularity.

Although using a lint was discussed in rust-lang#26009 I decided against this because it would require converting the AST output from the plugin back into an internal data-structure. Emitting the metadata from within the plugin prevents this double-handling. We also didn't identify this as the source of the failures last time, although something untoward was definitely happening... With that in mind I would like as much feedback as possible on this before it's merged, I don't want to break the bots again!

I've successfully built for my host architecture and I'm building an ARM cross-compiler now to test my assumptions about the various `CFG` variables. Despite the confusing name of `CFG_COMPILER_HOST_TRIPLE` it is actually the compile time target triple, as explained in `mk/target.mk`.

```
# This is the compile-time target-triple for the compiler. For the compiler at
# runtime, this should be considered the host-triple. More explanation for why
# this exists can be found on issue rust-lang#2400
export CFG_COMPILER_HOST_TRIPLE
```

CC @pnkfelix @brson @nrc @alexcrichton 

Closes rust-lang#25705, closes rust-lang#26009.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants