Static analysis: Error when falling though switch cases without an explicit "fallthrough" annotation

RESOLVED WONTFIX

Status

()

Core
Rewriting and Analysis
--
enhancement
RESOLVED WONTFIX
8 years ago
4 years ago

People

(Reporter: cjones, Assigned: Ehren Metcalfe)

Tracking

(Blocks: 1 bug)

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

I've coded this bug all the time

  switch(foo) {
  case FOO:
     doSomething();
     // oops! forgot to |break;|

  case BAR:
     doSomethingElse();  // CRASH!  bad fallthrough
  }

It would be great to require an explicit annotation for falling through switch cases.  Perhaps something like

  __attribute__(((user "fallthrough"))) inline static fallthrough() { }
  #define FALLTHROUGH()  fallthrough()

  switch (foo)
  case FOO:
     doSomething();
     // if i wanted to fallthrough, i would write
     // FALLTHROUGH();
   
  case BAR:
  ...
  }

The code above would trigger an error, because the fallthrough wasn't explicitly allowed.  Uncommenting "// FALLTHROUGH();" would suppress the error.

This is a pretty straightforward treehydra-CFG analysis.
Whiteboard: [good first bug]
(Assignee)

Updated

8 years ago
Assignee: nobody → ehren.m
Status: NEW → ASSIGNED
(Assignee)

Comment 1

8 years ago
Created attachment 426401 [details]
script

Here's a standalone script that's pretty much airtight. The only 'false positive' is |case '\0'| here: http://mxr.mozilla.org/mozilla-central/source/js/src/jskwgen.cpp#213 but this is kind of a fallthrough in disguise.

Also, whenever a case falls through to more than one case eg

case 0:
  x++
  /* Fallthrough */
case 1:
case 2:
case 3:
  x--;

the script will issue multiple warnings, but I suppose that's not too much of an issue. 

If we were willing to go forward and add this to a static checking build, would adding the 'fallthrough function' to nscore.h be sufficient?
(Assignee)

Comment 2

8 years ago
Created attachment 426403 [details]
list of fallthrough sites
I'm going through old "good first bugs" that have been inactive for a while. 
Chris, is this something you can help Ehren with? Do you think this bug could become a mentored bug?
This area isn't being worked on and I'm not available to mentor in it.
Whiteboard: [good first bug]
Our static analysis is built off of Clang these days, which has a warning for this sort of stuff (-Wimplicit-fallthrough) and an attribute to mark when it is explicitly desired ([[clang::fallthrough]]). We don't need a static analysis for this anymore, then.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.