Skip to content

Conversation

@rust-highfive
Copy link
Contributor

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

@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.

7 participants