coverage: Run-make regression test for #[derive(arbitrary::Arbitrary)]

[Zalathar]

This is a regression test for #141577 (comment), which resulted in llvm-cov producing the cryptic error message:

malformed instrumentation profile data: function name is empty

The repro we have is triggered by #[derive(arbitrary::Arbitrary)], and is difficult to minimize beyond that point. So instead of a coverage test, this PR adds a run-make test that builds against the real arbitrary crate, and then runs llvm-cov to verify that it doesn't crash.

---

I have manually verified that this would have caught the regression introduced by #144298.

r? compiler

[rustbot]

This PR modifies run-make tests.

cc @jieyouxu

The run-make-support library was changed

cc @jieyouxu

[Zalathar]

Ah, this will probably want needs-profiler-runtime.

[lqd]

It's quite uncommon to have tests with external dependencies. Is this only because it's still difficult to minimize? If so, I will minimize it.

[jieyouxu]

Remark: this is subject to the same consideration as #128733. Maybe at least pin this?

[Zalathar]

I was wondering how the other run-make cargo tests deal with these concerns, but I guess the answer is that they don't. 😱

[jieyouxu]

Yeah, unfortunately...

[folkertdev]

can't you run cargo expand on the input file to just expand the derive macro? Then the dependency is no longer needed.

[Zalathar]

can't you run cargo expand on the input file to just expand the derive macro? Then the dependency is no longer needed.

cargo expand doesn't preserve the authentic macro-expansion span hierarchy, which is a key part of what triggers the issue.

The expanded code would become plain Rust code, which would almost certainly not reproduce the problem.

[Zalathar]

It's quite uncommon to have tests with external dependencies. Is this only because it's still difficult to minimize? If so, I will minimize it.

Partly because it's difficult to minimize, and partly because I fear that a smaller repro would not actually be a very useful regression test if it ends up being overfitted to quirks of the current implementation.

The (coverage) test suite currently has approximately no coverage of real-world proc-macro usage, which is very often what encounters these obscure regressions. And it's not possible to capture real-world usage without dependencies like syn and quote.

I mainly wrote this test because #144530 (comment) made me worried about not being able to re-land #144298 without a test.

[lqd]

I fear that a smaller repro would not actually be a very useful regression test if it ends up being overfitted to quirks of the current implementation

That's the usual problem with all regression tests, and it's absolutely fine. We can't predict future changes and issues, so we try to prevent known ones. If it they reappear under slightly different conditions so be it, we'll fix that different issue. It happens all the time, and it's all good.

I mainly wrote this test because I didn't want #144530 (comment) threatening to hold up all future coverage work

We have tests for all issues where we can, that's unrelated to any future coverage work which I'm actually excited to see land sooner rather than later. That is why I r+ed the PR and didn't consider the lack of test blocking.

Big thanks for removing the annoying fuzzing setup in the original repro. I will make you a minimization ASAP.

[Zalathar]

It's quite uncommon to have tests with external dependencies. Is this only because it's still difficult to minimize? If so, I will minimize it.

Here's another tip that could help with minimization: after reapplying #144298, go to the bottom of fn coverage_ids_info, and assert !phys_counter_for_node.is_empty().

That will cause the buggy case to ICE the compiler, instead of having to do the extra steps with llvm-profdata and llvm-cov to see whether llvm-cov fails or not.

(Of course, it would be wise to verify the MCVE against llvm-cov to make sure it still reproduces the issue properly.)

[Zalathar]

@rustbot blocked

Waiting for an MCVE that could make this PR mostly obsolete.

[lqd]

proc macro:

#[proc_macro_derive(Arbitrary, attributes(arbitrary))]
pub fn derive_arbitrary(_: proc_macro::TokenStream) -> proc_macro::TokenStream {
   "impl Arbitrary for MyEnum {
        fn try_size_hint() -> Option<usize> {
            Some(0)?;
            None
        }
    }".parse().unwrap()
}

repro:

#[derive(derive_dependency::Arbitrary)]
enum MyEnum {}

fn main() {
    MyEnum::try_size_hint();
}

trait Arbitrary {
    fn try_size_hint() -> Option<usize>;
}

[Zalathar]

Thanks, that’s actually a more helpful repro than I was expecting. The main culprit seems to be the presence of ? in a proc-macro expansion. I was worried that it would involve some awkward user-span juggling.

I’ll get to work on adapting this into a coverage test, and verify it on my end.

[Zalathar]

Closing this in favour of #144616.

I would still like to get some more test coverage of “real-world” proc-macro expansions at some point, but that has its own obstacles such as #144571 (comment), so it's not a priority right now.