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

Make 'profile' a stable option #44673

Closed
wants to merge 2 commits into from

Conversation

marco-c
Copy link
Contributor

@marco-c marco-c commented Sep 18, 2017

@rust-highfive
Copy link
Collaborator

r? @eddyb

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

@marco-c
Copy link
Contributor Author

marco-c commented Sep 18, 2017

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned eddyb Sep 18, 2017
@kennytm
Copy link
Member

kennytm commented Sep 18, 2017

Just my opinion: Could we name it -C gcov-profile or -C profile=gcov? This allows for LLVM/clang's source-based coverage to use similarly named options if implemented in the future. https://clang.llvm.org/docs/SourceBasedCodeCoverage.html.

@alexcrichton
Copy link
Member

Thanks for the PR @marco-c! I'm realizing now that our documentation doesn't quite indicate this, but in general when we stabilize new features like this we require sign-off from the relevant subteam on the tracking issue beforehand. The issue here #42524, is tagged as T-dev-tools so you'll want to find a member of the dev-tools subteam and ask them to "propose FCP" on that issue. And then comments like @kennytm's can be discussed there as well!

@carols10cents carols10cents added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. labels Sep 18, 2017
@nrc
Copy link
Member

nrc commented Sep 18, 2017

I think this should be generalised a bit before stabilising and I like @kennytm's suggestion. @marco-c does that sound like a good idea to you?

Do I understand correctly that this is a feature where we depend entirely on LLVM for support? Are there stability worries there? (I.e., what happens if LLVM changes the output format or drops support altogether?)

Beyond these concerns, I think it is a good idea to stabilise this in some form. Let's start FCP now, but block on addressing those concerns.

@rfcbot fcp merge

@nrc
Copy link
Member

nrc commented Sep 18, 2017

@rfcbot concern naming
@rfcbot concern LLVM

@nrc
Copy link
Member

nrc commented Sep 18, 2017

@rfcbot concern documentation

We think this feature needs better documentation before it can be stabilised

@nrc
Copy link
Member

nrc commented Sep 18, 2017

@marco-c @kennytm, to confirm you're both using this feature in your coverage tools and it is working well? Any changes you think might be necessary?

@nrc
Copy link
Member

nrc commented Sep 18, 2017

After some more discussion, the dev-tools team thought this is probably not yet ready for stabilisation. We want to be in a place where coverage tools are working solidly with Rust code and are themselves getting some usage before we stabilise features which they depend on.

I think the best thing to do is to close this PR for now, and move discussion to the tracking issue (#42524).

@marco-c thanks for sending this PR and hopefully we can get some experience with coverage tools and then start the stabilisation process

@nrc nrc closed this Sep 18, 2017
@marco-c
Copy link
Contributor Author

marco-c commented Sep 19, 2017

OK. We are probably going to use this feature in Firefox automation with custom builds, so hopefully we will collect a lot of experience during the next few weeks.

@kennytm
Copy link
Member

kennytm commented Sep 19, 2017

@nrc It is not working well due to limitation of the GCOV format, debug info and how LLVM implements the pass, e.g.

  1. there's no column numbers
  2. macros — it needs something like RFC: Debuggable macro expansions rfcs#2117 so that it can see through error_chain!/bitflags! but ignore assert_eq!
  3. drop edges are treated the same as normal edges, often causing useless uncovered edges (in GCC exceptional edges carry a special flag, but LLVM is stuck with GCOV 4.2 format). Existing tutorial recommends -Z no-landing-pad or -C panic=aborts, but that means you can't write #[should_panic] tests at all. (As of 1.19 it is not possible to accurately measure unit test coverage #43410)
  4. there are often edges pointing to somewhere with no line info at all, which I don't know what is happening.
  5. Uninstantiated generic functions will simply be ignored.

I think LLVM's source-based coverage instrprof (#34701) is needed to solve 1, 3, 4 and 5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants