Page MenuHomePhabricator

Eliminates uninitialized warning for volatile variables.
AcceptedPublic

Authored by lethalantidote on Jan 10 2017, 5:46 PM.

Details

Reviewers
thakis
Summary

Eliminates uninitialized warning for volatile variables.

Event Timeline

lethalantidote retitled this revision from to Elliminates uninitialized warning for volitile varibles..
lethalantidote updated this object.
lethalantidote added a reviewer: thakis.
lethalantidote added a subscriber: cfe-commits.
thakis edited edge metadata.Jan 10 2017, 6:18 PM

Thanks! Can you add a test somewhere? grep -R W.*uninitialized test/Sema* will probably show you the existing test for this code. (Another technique: Comment some of the existing tests, run ninja check-clang, and see which tests start failing.)

You can run an individual test via bin/llvm-lit ../../llvm/tools/clang/test/Sema/mytest.cc.

clang/lib/Sema/AnalysisBasedWarnings.cpp
907

Should the check be in DiagUninitUse()? Or is there a reason this one should happen for volatiles?

lethalantidote marked an inline comment as done.
lethalantidote edited edge metadata.

Addresses thakis' comments and adds a test.

clang/lib/Sema/AnalysisBasedWarnings.cpp
907

Good point. Thanks for catching that. I hoped I understood this use case correctly, lemme know if the update makes sense.

Thanks for the test!

clang/lib/Sema/AnalysisBasedWarnings.cpp
699

What about the other cases in this switch? Does a volatile still warn in those cases? Should it? (Probably not?)

Right now the approach is to add an early return when the warning is emitted. Maybe this change should instead be somewhere where we compute if a var could be uninitialized, and that should always be false for volatile variables? Then future other warnings looking at that bit would get volatiles right for free.

907

Is it possible to test that both branches have this early return now?

lethalantidote marked an inline comment as done.

Addresses moving check further up, during analysis.
Adds test to check for sometimes branch. Please review.

Thanks, this is looking pretty good! From clicking around a bit on cs, do you think it's better to put the check where you have it, or is maybe http://llvm-cs.pcc.me.uk/tools/clang/lib/Analysis/UninitializedValues.cpp#36 more appropriate? I think having it where you have it means saying "we can reason about volatile vars, and we want to treat them as initialized" while the other spot means "we can't reason about volatile variables at all". I don't know which place is better (of someone else reading this knows, please speak up). If the other place makes your test fail, having the check where you have it is probably fine. Else I'd say that moving the check up is probably better.

clang/lib/Analysis/UninitializedValues.cpp
763 ↗(On Diff #84026)

No idea what should happen with volatile int x = x -- but I've never seen that in practice, so it probably doesn't matter too much either way.

Moves check in IsTracked().

So I tried the update you suggested (moving it up into IsTracked) and it seems to work and the reasoning makes sense to me.

thakis accepted this revision.Jan 13 2017, 12:41 PM
thakis edited edge metadata.

Looks great to me, thanks! Unless someone else shouts, I'll land this for you soon.

This revision is now accepted and ready to land.Jan 13 2017, 12:41 PM

Any updates on this?

hfinkel added a subscriber: hfinkel.Feb 9 2017, 7:19 AM

What's the motivation for this? The placement of a local volatile variable is still under the compiler's direction, and unless the address escapes, we still assume we can reason about its aliasing (and, thus, whether or not it is initialized).

What's the motivation for this? The placement of a local volatile variable is still under the compiler's direction, and unless the address escapes, we still assume we can reason about its aliasing (and, thus, whether or not it is initialized).

volatile means that the value can change at any time, which might initialize the variable, no?

joerg added a subscriber: joerg.Feb 10 2017, 2:41 PM

Please fix the spelling errors in the titel / summary before commit.

I somewhat agree with Hal -- I think this is too aggressive. Common use cases for local volatile include atomic ops or returns-twice functions like setjmp/longjmp.
Disabling the warning in those cases has a high chance of hiding real problems. I would find it much more useful to move this case into a warning subgroup, so that
it can be selectively disabled by command line or pragma.

lethalantidote retitled this revision from Elliminates uninitialized warning for volitile varibles. to Eliminates uninitialized warning for volatile variables..Feb 10 2017, 2:45 PM
lethalantidote edited the summary of this revision. (Show Details)

Is there any subgroup that one could suggest for this warning to fall under?