Page MenuHomePhabricator

Let -Wweak-template-vtables warn on implicit instantiations
Needs ReviewPublic

Authored by aaronpuchert on Apr 29 2021, 1:30 PM.

Details

Reviewers
dblaikie
rsmith
Summary

Implicit instantiations of template classes with virtual methods might
cause the vtable (and all virtual functions) to be instantiated and
emitted in multiple translation units. An explicit instantiation
declaration next to the template definition would ensure that only the
translation unit with the corresponding definition emits the vtable.

There are two places where we could emit the warning, on the class or
the implicit instantiation. I decided for emitting the warning on the
class, because that's likely where the fix would need to be. (Unless the
programmer decides to drop the instantiation or give a template argument
internal linkage.)

Fixes PR18733.

Diff Detail

Unit TestsFailed

TimeTest
2,230 msx64 debian > libarcher.races::lock-unrelated.c
Script: -- : 'RUN: at line 13'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/lock-unrelated.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-unrelated.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/deflake.bash /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-unrelated.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-unrelated.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/lock-unrelated.c

Event Timeline

aaronpuchert requested review of this revision.Apr 29 2021, 1:30 PM
aaronpuchert created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2021, 1:30 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Along time ago Clang had a fairly strong aversion to implementing "off by default" warnings (though clearly weak-vtables was an exception to that - it's a pretty esoteric warning even at the best of times/without this special case) because they would tend to go unused and unmaintained. I'm sort of inclined towards this subset of the warning (either the poorly implemented one originally, or this version) being that sort of category, and that it'd be better to delete it.

Honestly in retrospect I think it was a bug in the warning that should've been fixed by not warning in this case, rather than splitting it out into its own warning group. The warning was originally written to ignore implicit template instantiations - and should've ignored explicit instantiations too, I think.

@doug.gregor - any interest in revisiting this (there's some discussion on the linked bug, which links back to a discussion we had many many years ago... )?

@aaronpuchert - do you have plans to use this warning, if it's implemented/changed in this way?
@respindola - do you have any interest in using this warning if it were implemented/changed in this way?

Along time ago Clang had a fairly strong aversion to implementing "off by default" warnings ([...]) because they would tend to go unused and unmaintained.

That was a valid reason, but now there are code bases that work with -Weverything and disable the warnings they are not interested in.

I'm sort of inclined towards this subset of the warning (either the poorly implemented one originally, or this version) being that sort of category, and that it'd be better to delete it.

We have other warnings that are basically just about "cleaner" object files, like -Wmissing-prototypes or -Wmissing-variable-declarations. (These enforce adding static on functions/variables not previously declared. Though together with -Wunused-{function,variable} one could find more unused functions/variables, which could be seen as improving code quality.)

The warning was originally written to ignore implicit template instantiations - and should've ignored explicit instantiations too, I think.

Looking at the warning name I think you're right. There is no way to make the vtables strong. But I don't think anybody cares about this technicality, the warning is there to reduce compile time and object file sizes.

In fact it's probably not so much about emitting the vtable, but about emitting all virtual functions, even if they end up unused. In the template case this also implies instantiation.

@aaronpuchert - do you have plans to use this warning, if it's implemented/changed in this way?

Let's say that I'm considering it. I'll need to see how often this fires and whether the explicit instantiations make sense to me.

Along time ago Clang had a fairly strong aversion to implementing "off by default" warnings ([...]) because they would tend to go unused and unmaintained.

That was a valid reason, but now there are code bases that work with -Weverything and disable the warnings they are not interested in.

Sure - though that's likely a pretty small proportion of the userbase/not something we encourage/support/etc. So still not necessarily getting a lot of value from new warnings - especially this one, I'd imagine (I'd expect most users would turn this warning off and never think about it again - the intersection of people using -Weverything (small population) and people who will want to put explicit instantiations of all their templates with vtables (small as well) seems vanishingly small.

I'm sort of inclined towards this subset of the warning (either the poorly implemented one originally, or this version) being that sort of category, and that it'd be better to delete it.

We have other warnings that are basically just about "cleaner" object files, like -Wmissing-prototypes or -Wmissing-variable-declarations. (These enforce adding static on functions/variables not previously declared. Though together with -Wunused-{function,variable} one could find more unused functions/variables, which could be seen as improving code quality.)

Missing declarations can be helpful for finding errors in source code earlier, rather than getting harder to understand linker errors (if you miss "const" when defining a function and create an overload instead of defining the thing you declared in the header, for instance).

And at least both those flags were probably implemented for GCC compatibility, which I don't think applies to this warning.

The warning was originally written to ignore implicit template instantiations - and should've ignored explicit instantiations too, I think.

Looking at the warning name I think you're right. There is no way to make the vtables strong. But I don't think anybody cares about this technicality, the warning is there to reduce compile time and object file sizes.

In fact it's probably not so much about emitting the vtable, but about emitting all virtual functions, even if they end up unused. In the template case this also implies instantiation.

@aaronpuchert - do you have plans to use this warning, if it's implemented/changed in this way?

Let's say that I'm considering it. I'll need to see how often this fires and whether the explicit instantiations make sense to me.

I think it'd be good to gather that data first before committing it to Clang - that's usually what we try to do to justify the warning.

I think it'd be good to gather that data first before committing it to Clang - that's usually what we try to do to justify the warning.

Well, except that this isn't a new warning. But yeah, it does make sense to look at some actual code bases.

  • Fix punctuation in expected warning message.
  • Fix test failure: the instantiated class might not be a template, it could also be the member of another template.

So I tried this in two code bases, both of which don't have -Wweak-vtables enabled though.

In the first (~500 TUs) there were a couple of interesting finds. But they are hard to fix, even where explicit instantiations were already there: the corresponding instantiation declarations would require additional includes in the headers because many of the templates are constrained, and concepts don't work well with forward declarations. That's also why we had to disable -Wundefined-func-templates there, which is otherwise a useful warning.

In the second code base (~30,000 TUs) it looks a lot more useful. There are many occurrences, but deduplicating and sorting by number of files they occur in finds a couple of templates where explicit instantiation could improve compile times and build sizes. To give some examples, its own standard library has instantiations basic_ios<char, char_traits<char>> plus the wchar_t equivalent or basic_ostream<char, char_traits<char>> and so on coming up in most TUs. Instantiating them explicitly would be natural and likely beneficial.

Overall it's not a warning that I would enable in regular builds, but rather like -Wweak-vtables collect the most common occurrences in special runs and do something about them. (The total number of warnings, not deduplicated, runs into the millions for both warnings on the second code base.)

So I tried this in two code bases, both of which don't have -Wweak-vtables enabled though.

In the first (~500 TUs) there were a couple of interesting finds. But they are hard to fix, even where explicit instantiations were already there: the corresponding instantiation declarations would require additional includes in the headers because many of the templates are constrained, and concepts don't work well with forward declarations. That's also why we had to disable -Wundefined-func-templates there, which is otherwise a useful warning.

In the second code base (~30,000 TUs) it looks a lot more useful. There are many occurrences, but deduplicating and sorting by number of files they occur in finds a couple of templates where explicit instantiation could improve compile times and build sizes. To give some examples, its own standard library has instantiations basic_ios<char, char_traits<char>> plus the wchar_t equivalent or basic_ostream<char, char_traits<char>> and so on coming up in most TUs. Instantiating them explicitly would be natural and likely beneficial.

Out of curiosity - have you tried it & measured any significant improvement/value in build times/sizes/etc?

Overall it's not a warning that I would enable in regular builds, but rather like -Wweak-vtables collect the most common occurrences in special runs and do something about them. (The total number of warnings, not deduplicated, runs into the millions for both warnings on the second code base.)

That sort of use case sounds more like what we'd use clang-tidy for these days.

This doesn't sound especially compelling for a warning (& still seems pretty much not what the original weak-vtables warning was intending to do for templates - and an inversion of the weak-template-vtables (& so I think, if we are going to have this new thing as a warning, it should have a different name and the existing name should be removed)).

I still really think the best thing is to delete the existing weak-template-vtables warning entirely.

Out of curiosity - have you tried it & measured any significant improvement/value in build times/sizes/etc?

No, I fear that would take too much time. (Not so much the benchmarking, but making a number of fixes that can be expected to make a dent.)

This doesn't sound especially compelling for a warning (& still seems pretty much not what the original weak-vtables warning was intending to do for templates - and an inversion of the weak-template-vtables (& so I think, if we are going to have this new thing as a warning, it should have a different name and the existing name should be removed)).

I still really think the best thing is to delete the existing weak-template-vtables warning entirely.

I still don't understand the difference. You can of course argue that explicit instantiations are still weak, but I'd be curious why anyone would care about that. What is the reason behind -Wweak-vtables if not compile time or build size reductions? I can just guess, because the original bug 6116 doesn't state a reason.

Out of curiosity - have you tried it & measured any significant improvement/value in build times/sizes/etc?

No, I fear that would take too much time. (Not so much the benchmarking, but making a number of fixes that can be expected to make a dent.)

Makes it hard to justify the complexity in the compiler if it's hard to justify/support the value of the warning.

This doesn't sound especially compelling for a warning (& still seems pretty much not what the original weak-vtables warning was intending to do for templates - and an inversion of the weak-template-vtables (& so I think, if we are going to have this new thing as a warning, it should have a different name and the existing name should be removed)).

I still really think the best thing is to delete the existing weak-template-vtables warning entirely.

I still don't understand the difference. You can of course argue that explicit instantiations are still weak, but I'd be curious why anyone would care about that. What is the reason behind -Wweak-vtables if not compile time or build size reductions? I can just guess, because the original bug 6116 doesn't state a reason.

I believe it's compile time/build time, yes - but yeah, it's pretty questionable/suspect. LLVM's the only project I know of with it as a coding convention/guideline/rule - and even we haven't even remotely tried to enforce it. (& when I did do a bit of work to add more key functions people reasonably questioned the value of them - and I didn't really have data to support it, I could only point to the fact that I was implementing the stated policy/style guide)

Makes it hard to justify the complexity in the compiler if it's hard to justify/support the value of the warning.

The complexity for -Wweak-template-vtables is just 10 lines of code. We're just using information that's already there.

I believe it's compile time/build time, yes - but yeah, it's pretty questionable/suspect. LLVM's the only project I know of with it as a coding convention/guideline/rule - and even we haven't even remotely tried to enforce it. (& when I did do a bit of work to add more key functions people reasonably questioned the value of them - and I didn't really have data to support it, I could only point to the fact that I was implementing the stated policy/style guide)

Just to make sure I got this right, you're suggesting to remove both warnings? I agree that their value is pretty limited and it would be consistent. I'm just not sure if maybe someone is using -Wweak-vtables (and would consider using -Wweak-template-vtables if it did the right thing).

Makes it hard to justify the complexity in the compiler if it's hard to justify/support the value of the warning.

The complexity for -Wweak-template-vtables is just 10 lines of code. We're just using information that's already there.

I believe it's compile time/build time, yes - but yeah, it's pretty questionable/suspect. LLVM's the only project I know of with it as a coding convention/guideline/rule - and even we haven't even remotely tried to enforce it. (& when I did do a bit of work to add more key functions people reasonably questioned the value of them - and I didn't really have data to support it, I could only point to the fact that I was implementing the stated policy/style guide)

Just to make sure I got this right, you're suggesting to remove both warnings?

Right - to remove -Wweak-template-vtable in its entirety. The original implementation explicitly didn't warn on implicit instantiations and I think the fact that it warned on explicit instantiations is more bug than feature - and we should treat it that way.

Right - to remove -Wweak-template-vtable in its entirety. The original implementation explicitly didn't warn on implicit instantiations and I think the fact that it warned on explicit instantiations is more bug than feature - and we should treat it that way.

What about -Wweak-vtables, what's the reasoning for keeping that?

Right - to remove -Wweak-template-vtable in its entirety. The original implementation explicitly didn't warn on implicit instantiations and I think the fact that it warned on explicit instantiations is more bug than feature - and we should treat it that way.

What about -Wweak-vtables, what's the reasoning for keeping that?

That it doesn't seem to have major issues to me - so I'm willing to leave it there, even if it's questionable as to whether anyone uses it.