Page MenuHomePhabricator

Adds -Wrange-loop-analysis to -Wall
ClosedPublic

Authored by Mordante on Oct 12 2019, 8:53 AM.

Diff Detail

Event Timeline

Mordante created this revision.Oct 12 2019, 8:53 AM
Mordante updated this revision to Diff 226539.Oct 26 2019, 4:46 AM

Adds a test as requested by @xbolva00 .

xbolva00 accepted this revision.Oct 26 2019, 5:06 AM
This revision is now accepted and ready to land.Oct 26 2019, 5:06 AM

Does this analysis require CFG support or have a high false positive rate? We typically keep those out of -Wall because of performance concerns, so I am wondering if the diagnostic was kept out of -Wall for a reason.

xbolva00 requested changes to this revision.Oct 28 2019, 9:40 AM

Does this analysis require CFG support

https://reviews.llvm.org/D69292 also requires CFG support.

Generally, is CFG support ok for -Wall if the warning has few false positives?

I am wondering if the diagnostic was kept out of -Wall for a reason.

Yes, it would be good to find why this was disabled (never enabled?).

@Mordante can you compile LLVM itself with this patch?

This revision now requires changes to proceed.Oct 28 2019, 9:40 AM

Does this analysis require CFG support

https://reviews.llvm.org/D69292 also requires CFG support.

Yes, it does.

Generally, is CFG support ok for -Wall if the warning has few false positives?

I think it also depends on the performance of the diagnostic.

I am wondering if the diagnostic was kept out of -Wall for a reason.

Yes, it would be good to find why this was disabled (never enabled?).

@Mordante can you compile LLVM itself with this patch?

I'd be curious to know how the timing changes between compilations as well; does this regress performance considerably?

Does this analysis require CFG support

https://reviews.llvm.org/D69292 also requires CFG support.

Yes, it does.

Generally, is CFG support ok for -Wall if the warning has few false positives?

I think it also depends on the performance of the diagnostic.

I am wondering if the diagnostic was kept out of -Wall for a reason.

Yes, it would be good to find why this was disabled (never enabled?).

I don't know, @rtrieu originally added the warnings, maybe he can answer that question.

@Mordante can you compile LLVM itself with this patch?

I compiled LLVM and Clang and it adds 145 unique warnings. I wouldn't mind to fix those and the other subprojects, so I can test whether the tests have false positives.
Am I correct to assume these patches all need to be reviewed?

I'd be curious to know how the timing changes between compilations as well; does this regress performance considerably?

I did a partial build of LLVM the timings are quite similar:

Before:
real 7m44.918s
user 14m41.052s
sys 0m16.560s

After:
real 7m52.934s
user 14m39.968s
sys 0m16.932s

I compiled LLVM and Clang and it adds 145 unique warnings. I wouldn't mind to fix those and the other subprojects, so I can test whether the tests have false positives.
Am I correct to assume these patches all need to be reviewed?

Can you upload the list with new warnings somewhere? How many false positives?

I dont think you have to fix them as a prerequisite - buildbots do not use -Werror.

I haven't reviewed the list [1] properly so I don't know the number of false positives yet.

[1] http://paste.debian.net/1111806/

aaron.ballman accepted this revision.Nov 7 2019, 2:29 AM

I spot-checked the list and definitely saw some true positives in there. I did not exhaustively check the list, however. The timing numbers look reasonable enough as well. Unless someone finds an issue with the fp rate that I've missed, I think this is reasonable.

Thanks for the review. I also looked at some of the new warnings and I'm preparing patches to fix them.

xbolva00 accepted this revision.Nov 7 2019, 12:39 PM

Formally unblocking this

This revision is now accepted and ready to land.Nov 7 2019, 12:39 PM

Formally unblocking this

Thanks. I'll first want to reduce the number of warnings before committing this patch.

I think you can commit it, you already fixed some warnings.

xbolva00 added a comment.EditedNov 23 2019, 12:53 PM

I think you can commit it, you already fixed some warnings.

Oh, I hit this (-Werror bots) hard :D Some bots use -Werror. So when build is warning-free, then please commit this patch.

I think you can commit it, you already fixed some warnings.

Oh, I hit this (-Werror bots) hard :D Some bots use -Werror. So when build is warning-free, then please commit this patch.

I will, a bit busy at the moment but will look at fixing the warnings before committing.

All is fixed now?

All is fixed now?

Not yet :-(
I ran into a false positive with rvalue references, will post a code fix for it and then I'll look into posting more error fixes.

This revision was automatically updated to reflect the committed changes.
denik added a subscriber: denik.Jan 17 2020, 12:03 PM

We are hitting the warning in template code with bool and loop like this: "for (const auto& element : value)" where element is bool from template. But not always.
When it is bool the warning triggers:
error: loop variable 'element' is always a copy because the range of type 'const std::vector<bool, allocator<bool> >' does not return a reference [-Werror,-Wrange-loop-analysis]

However, the template type can also use non-bools and if I remove the reference I hit alternative case where the warning complains about copy operation of reference types:
error: loop variable 'element' of type 'const std::1::basic_string<char>' creates a copy from type 'const std::1::basic_string<char>' [-Werror,-Wrange-loop-analysis]
for (const auto element : value):
use reference type 'const std::1::tuple<unsigned int, bool, std::1::vector<unsigned char, std::__1::allocator<unsigned char> > > &' to prevent copying

So there is no way to satisfy the warning other than disabling it.

Thanks for your report. I think it's similar to https://bugs.llvm.org/show_bug.cgi?id=44556. I'll look for a fix.

I proposed D73007 as fix.