RFR: 8003310: Enable -Wunused when compiling with GCC (original) (raw)

Mikael Vidstedt mikael.vidstedt at oracle.com
Thu Mar 28 09:40:34 PDT 2013

I've been waiting for Joe's changes to bubble up so as to not add a conflict unnecessarily, and since they're now in hotspot-rt I have uploaded a webrev for what I hope is a final time knocking on wood. I would appreciate a quick check to see that everything still looks fine:

http://cr.openjdk.java.net/~mikael/webrevs/8003310/webrev.08/webrev/

The only relevant changes from the last webrev should be:

make/linux/makefiles/gcc.make:

src/share/vm/runtime/arguments.cpp:

Thanks, Mikael

On 2/21/2013 7:16 PM, David Holmes wrote:

One final comment - please liaise with Joe Provino who is adding -Wundef so we can get a consistent approach as to where these -Wxxx flags get added. :)

(Don't know what I was looking at with the tracelocking stuff - it ends up as an empty method.) David On 22/02/2013 6:00 AM, Mikael Vidstedt wrote:

On 2013-02-20 10:04, Coleen Phillimore wrote: On 2/20/2013 12:33 PM, Mikael Vidstedt wrote:

On 2/19/2013 12:01 PM, Christian Thalinger wrote: On Feb 18, 2013, at 3:14 PM, Mikael Vidstedt <mikael.vidstedt at oracle.com> wrote:

A really good control question, thanks a lot for asking!

As a matter of fact these changes are basically just doing what the compiler already does for us, so it's more about cleaning up the source code than reducing the size of the binary. So in theory these changes should have no impact on the binary size at all, but it actually turns out they do. Very specifically, the fact that I moved the samepage() function from being duplicated in the three os.cpp files to having it be in globalDefinitions.cpp makes the binary grow a few bytes (54 bytes to be specific). The reason is a bit subtle: The samepage() function is (was) static in the respective os*.cpp files. They are only ever used when the Verbose flag is true, and furthermore the Verbose option is a develop only flag, which means it is hardcoded to false in product. The compiler knows that's the case and eliminates the samepage() function completely. Since I moved it to globalDefinitions.cpp there's no way easy for the compiler to know that it is not being used, so it will actually keep the function. Unless there are strong opinions I'm not going to do anything about this. However, this made me question my earlier experiments with using the const bool = false construct in constantPool.cpp, because after all that is the exact same pattern. And it turns out that I must have done something wrong when I was performing the experiments, because when I do the same thing again now it turns out the compiler actually does deadcode eliminate the debug-only functions. So I take everything back and conclude that const bool = false is indeed a great way to make sure the debug code does not rot over time, and that the product binary will not contain the dead functions. Sorry for any confusion this may have caused. With all that in mind, here's another version of the webrev: http://cr.openjdk.java.net/~mikael/webrevs/8003310/webrev04/webrev/ src/share/vm/interpreter/interpreterRuntime.cpp: I suppose that #if 0 is to keep that code for...? Could we add a comment why we keep it? This was by "popular request" (from David Holmes) :) I personally don't know if and how this is being used. Note that ObjectSynchronizer::tracelocking is PRODUCTRETURN and the non-product implementation in synchronizer.cpp is empty (modulo the comment saying "Don't know what to do here"), so one can question the value of keeping the any of the tracelocking functions, but they may be someone's favorite debugging tool. Until proven differently I will leave it in there for now. If somebody can help me provide a useful comment about how the code is actually being used then I will certainly add it, otherwise I'll keep this as it is. If I remember correctly, this tracelocking code was associated with a flag that we removed a long time ago. I don't think if we want to implement TraceLocking we'd use this function. My vote is that it and the ObjectMonitor:: version should be removed. Thanks Coleen, that makes sense. I've prepared a final version of the webrev with the tracelocking methods removed too: http://cr.openjdk.java.net/~mikael/webrevs/8003310/webrev.06/webrev/ Any final comments? Cheers, Mikael Coleen src/share/vm/oops/constantPool.cpp: So we use: + const bool debugcpool = false; but we still have the DBG macro. Can't we have a static debug method that takes all printf arguments and prints them? The debugcpool should make that method empty and the compiler can remove it. I really start to hate all these macros... I share your macro hate and I actually played around with removing the macros just as you say. There is one small problem with doing it that way, and that's the fact that a few of the printf:s print symbols names, and as part of that they need to create utf8 strings using sym->asutf8(). The compiler can unfortunately not know that that function is for all intents and purposes a no-op, so it will keep the call even in product meaning increased binary size and it will also add to the runtime since it actually performs the call and creates the utf8 string. So I'm going to keep the macros for now. I also moved the clampaddressinpage to the globalDefinitions.hpp header file, meaning it will be inlined and dead code eliminated. New webrev here: http://cr.openjdk.java.net/~mikael/webrevs/8003310/webrev.05/webrev/ Final comments? Cheers, Mikael

More information about the hotspot-dev mailing list