Bug 136616 – Introduce COMPILER_QUIRK(CONSIDERS_UNREACHABLE_CODE) and use it

• Component: New Bugs
• Status: RESOLVED FIXED

[Maciej Stachowiak]

Introduce COMPILER_QUIRK(CONSIDERS_UNREACHABLE_CODE) and use it

[Maciej Stachowiak]

Created attachment 237761 [details]
Patch

[Maciej Stachowiak]

Testing this for arm32/64 to see if it fixes the build issues.

[Darin Adler]

Comment on attachment 237761 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=237761&action=review

I find the name of this a bit oblique and even possibly ambiguous.

a) To “consider unreachable code” could be to correctly consider whether code is unreachable and complain if any such code is included.

b) Or to “consider unreachable code” could be to compile unreachable code, as a matter of policy or even compiler ignorance, and evaluate the properties of that code as if it was reachable, warning about missing return statements and uninitialized variables and such.

I think you mean (b), but I had thought you meant (a).

What’s undeniable is that it is indeed a compiler quirk. And that this name is long enough that we could revise it with a global replace.

> Source/JavaScriptCore/jsc.cpp:368
> -#if !COMPILER(CLANG) && !COMPILER(MSVC)
> +#if COMPILER_QUIRK(CONSIDERS_UNREACHABLE_CODE)
>          return true;
>  #endif

We will now compile in this return statement on MSVC; is that an intentional change?

[Maciej Stachowiak]

(In reply to comment #3)
> (From update of attachment 237761 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237761&action=review

> 
> > Source/JavaScriptCore/jsc.cpp:368
> > -#if !COMPILER(CLANG) && !COMPILER(MSVC)
> > +#if COMPILER_QUIRK(CONSIDERS_UNREACHABLE_CODE)
> >          return true;
> >  #endif
> 
> We will now compile in this return statement on MSVC; is that an intentional change?

I'm not sure if MSVC requires excluding this code, or just doesn't require including it. My plan was for the EWS bot to tell me if this change is wrong.

[Maciej Stachowiak]

Committed r173370: <http://trac.webkit.org/changeset/173370>

[Filip Pizlo]

This is really gross.  I think it's a step in the wrong direction to introduce #if-based control flow all over the place.

Isn't there some alternative that doesn't involve uglifying the code?

[Maciej Stachowiak]

(In reply to comment #6)
> This is really gross.  I think it's a step in the wrong direction to introduce #if-based control flow all over the place.
> 
> Isn't there some alternative that doesn't involve uglifying the code?

Here's the constraints:

- Some non-clang compilers (GCC and perhaps MSVC, don't know for sure) analyze dead code paths when determining things like variables that may be uninitialized. So if we leave the #if'd code out, we get warnings in those.

- New clang has a warning for unreachable code which seems kinda useful in theory (though not sure it has caught any real issues for us yet). It will give warnings if we leave the #if'd code in.

Alternatives I can think of:
- Drop support for non-clang compilers (probably not practical if MSVC is one of the compilers to spuriously warn without this code).
- Lower warning levels on gcc so it doesn't check for uninitialized variables at all.
- Turn clang's unreachable code warning back off.

I can't think of a solution that keeps all the warnings on and avoids #ifs. I guess one possibility is we could make RELEASE_ASSERT() take a block as an extra parameter, which would hide the #if in RELEASE_ASSERT but would still be ugly.


I'm not sure if any of these is ideal. I have nothing against turning the warning off the warning. I just tried to fix the build without expressing an opinion on that.

[Filip Pizlo]

(In reply to comment #7)
> (In reply to comment #6)
> > This is really gross.  I think it's a step in the wrong direction to introduce #if-based control flow all over the place.
> > 
> > Isn't there some alternative that doesn't involve uglifying the code?
> 
> Here's the constraints:
> 
> - Some non-clang compilers (GCC and perhaps MSVC, don't know for sure) analyze dead code paths when determining things like variables that may be uninitialized. So if we leave the #if'd code out, we get warnings in those.
> 
> - New clang has a warning for unreachable code which seems kinda useful in theory (though not sure it has caught any real issues for us yet). It will give warnings if we leave the #if'd code in.
> 
> Alternatives I can think of:
> - Drop support for non-clang compilers (probably not practical if MSVC is one of the compilers to spuriously warn without this code).
> - Lower warning levels on gcc so it doesn't check for uninitialized variables at all.
> - Turn clang's unreachable code warning back off.

I suspect that this last option is better than uglification.

> 
> I can't think of a solution that keeps all the warnings on and avoids #ifs. I guess one possibility is we could make RELEASE_ASSERT() take a block as an extra parameter, which would hide the #if in RELEASE_ASSERT but would still be ugly.
> 
> 
> I'm not sure if any of these is ideal. I have nothing against turning the warning off the warning. I just tried to fix the build without expressing an opinion on that.

Does the unreachable code warning require guarding things like a "break" or "return" after a "CRASH()"?  It seems from your patch that it doesn't; for example:

     default:
         RELEASE_ASSERT_NOT_REACHED();
+#if COMPILER_QUIRK(CONSIDERS_UNREACHABLE_CODE)
         clippedValue = 0; // Make some compilers, and mhahnenberg, happy.
+#endif
         break;
     }

If this is true, then I propose a new macro, which I will call DEAD_DEF() for now because I haven't thought of a better name, yet:

#if COMPILER_QUIRK(CONSIDERS_UNREACHABLE_CODE)
#define DEAD_DEF(definition) do { definition } while (false)
#else
#define DEAD_DEF(definition) do { } while (false)
#endif

Then the change above, from DFGOSRExitCompilerCommon.cpp, would just be:

     default:
         RELEASE_ASSERT_NOT_REACHED();
-         clippedValue = 0; // Make some compilers, and mhahnenberg, happy.
+         DEAD_DEF(clippedValue = 0); // Make some compilers, and mhahnenberg, happy.
         break;
     }

[Maciej Stachowiak]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > This is really gross.  I think it's a step in the wrong direction to introduce #if-based control flow all over the place.
> > > 
> > > Isn't there some alternative that doesn't involve uglifying the code?
> > 
> > Here's the constraints:
> > 
> > - Some non-clang compilers (GCC and perhaps MSVC, don't know for sure) analyze dead code paths when determining things like variables that may be uninitialized. So if we leave the #if'd code out, we get warnings in those.
> > 
> > - New clang has a warning for unreachable code which seems kinda useful in theory (though not sure it has caught any real issues for us yet). It will give warnings if we leave the #if'd code in.
> > 
> > Alternatives I can think of:
> > - Drop support for non-clang compilers (probably not practical if MSVC is one of the compilers to spuriously warn without this code).
> > - Lower warning levels on gcc so it doesn't check for uninitialized variables at all.
> > - Turn clang's unreachable code warning back off.
> 
> I suspect that this last option is better than uglification.
> 
> > 
> > I can't think of a solution that keeps all the warnings on and avoids #ifs. I guess one possibility is we could make RELEASE_ASSERT() take a block as an extra parameter, which would hide the #if in RELEASE_ASSERT but would still be ugly.
> > 
> > 
> > I'm not sure if any of these is ideal. I have nothing against turning the warning off the warning. I just tried to fix the build without expressing an opinion on that.
> 
> Does the unreachable code warning require guarding things like a "break" or "return" after a "CRASH()"?  It seems from your patch that it doesn't; for example:
> 
>      default:
>          RELEASE_ASSERT_NOT_REACHED();
> +#if COMPILER_QUIRK(CONSIDERS_UNREACHABLE_CODE)
>          clippedValue = 0; // Make some compilers, and mhahnenberg, happy.
> +#endif
>          break;
>      }
> 
> If this is true, then I propose a new macro, which I will call DEAD_DEF() for now because I haven't thought of a better name, yet:
> 
> #if COMPILER_QUIRK(CONSIDERS_UNREACHABLE_CODE)
> #define DEAD_DEF(definition) do { definition } while (false)
> #else
> #define DEAD_DEF(definition) do { } while (false)
> #endif
> 
> Then the change above, from DFGOSRExitCompilerCommon.cpp, would just be:
> 
>      default:
>          RELEASE_ASSERT_NOT_REACHED();
> -         clippedValue = 0; // Make some compilers, and mhahnenberg, happy.
> +         DEAD_DEF(clippedValue = 0); // Make some compilers, and mhahnenberg, happy.
>          break;
>      }

That is totally reasonable and I considered doing it that way. In some cases there are returns inside the #if's, but that is compatible with this approach AFAICT.

Instead of DEAD_DEF I would just call the macro UNREACHABLE() (or maybe DEAD()). The statement need not be a definition or assignment. I can post a patch along those lines if you liked it.

[Filip Pizlo]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #6)
> > > > This is really gross.  I think it's a step in the wrong direction to introduce #if-based control flow all over the place.
> > > > 
> > > > Isn't there some alternative that doesn't involve uglifying the code?
> > > 
> > > Here's the constraints:
> > > 
> > > - Some non-clang compilers (GCC and perhaps MSVC, don't know for sure) analyze dead code paths when determining things like variables that may be uninitialized. So if we leave the #if'd code out, we get warnings in those.
> > > 
> > > - New clang has a warning for unreachable code which seems kinda useful in theory (though not sure it has caught any real issues for us yet). It will give warnings if we leave the #if'd code in.
> > > 
> > > Alternatives I can think of:
> > > - Drop support for non-clang compilers (probably not practical if MSVC is one of the compilers to spuriously warn without this code).
> > > - Lower warning levels on gcc so it doesn't check for uninitialized variables at all.
> > > - Turn clang's unreachable code warning back off.
> > 
> > I suspect that this last option is better than uglification.
> > 
> > > 
> > > I can't think of a solution that keeps all the warnings on and avoids #ifs. I guess one possibility is we could make RELEASE_ASSERT() take a block as an extra parameter, which would hide the #if in RELEASE_ASSERT but would still be ugly.
> > > 
> > > 
> > > I'm not sure if any of these is ideal. I have nothing against turning the warning off the warning. I just tried to fix the build without expressing an opinion on that.
> > 
> > Does the unreachable code warning require guarding things like a "break" or "return" after a "CRASH()"?  It seems from your patch that it doesn't; for example:
> > 
> >      default:
> >          RELEASE_ASSERT_NOT_REACHED();
> > +#if COMPILER_QUIRK(CONSIDERS_UNREACHABLE_CODE)
> >          clippedValue = 0; // Make some compilers, and mhahnenberg, happy.
> > +#endif
> >          break;
> >      }
> > 
> > If this is true, then I propose a new macro, which I will call DEAD_DEF() for now because I haven't thought of a better name, yet:
> > 
> > #if COMPILER_QUIRK(CONSIDERS_UNREACHABLE_CODE)
> > #define DEAD_DEF(definition) do { definition } while (false)
> > #else
> > #define DEAD_DEF(definition) do { } while (false)
> > #endif
> > 
> > Then the change above, from DFGOSRExitCompilerCommon.cpp, would just be:
> > 
> >      default:
> >          RELEASE_ASSERT_NOT_REACHED();
> > -         clippedValue = 0; // Make some compilers, and mhahnenberg, happy.
> > +         DEAD_DEF(clippedValue = 0); // Make some compilers, and mhahnenberg, happy.
> >          break;
> >      }
> 
> That is totally reasonable and I considered doing it that way. In some cases there are returns inside the #if's, but that is compatible with this approach AFAICT.
> 
> Instead of DEAD_DEF I would just call the macro UNREACHABLE() (or maybe DEAD()). The statement need not be a definition or assignment. I can post a patch along those lines if you liked it.

Yes, this sounds great!

[Maciej Stachowiak]

(In reply to comment #10)
> (In reply to comment #9)

> > Instead of DEAD_DEF I would just call the macro UNREACHABLE() (or maybe DEAD()). The statement need not be a definition or assignment. I can post a patch along those lines if you liked it.
> 
> Yes, this sounds great!

https://bugs.webkit.org/show_bug.cgi?id=136679