When compiling Pike with clang I get massive amounts of warnings for unused variables. This detracts quite a bit from finding more serious errors and I've got a patch ready to remove them. However, before pushing that I'd like to hear if people have objections to changing declarations like in these examples:
- static void exit_memory(struct object *o) + static void exit_memory(struct object *UNUSED(o))
and
- static int got_udp_event (struct fd_callback_box *box, int event) + static int got_udp_event (struct fd_callback_box *box, int DEBUGUSED(event))
I assume it's something that expands to UNUSED only if not making a debug build, since the use of the variable is inside #ifdef DEBUG.
(For gcc this is a bit overkill since you can still use a varabile declared as __attribute__((unused)) without getting a warning, but maybe it is needed for clang?)
The macro I'm borrowing renames variables to prevent accidental use. There are still places where variable usage is conditioned on other defines (e.g. POLL_DEBUG) and I'll leave them unchanged instead of creating a more complex wrapper.
Pushed now. If anything breaks please let me know.
This cleanup was kickstarted when I tried to compile 7.9 with Clang. I still see wrong execution in e.g. integer/bignum handling so I hope to have more patches in the coming days. One of my other findings was the commit aabfb4f04c5 (precompile.pike) which probably is a candidate for 7.8 backport as well but I'll let Grubba weigh in on that.
One issue I run into is that the definition of UNUSED clashes with a similar definition used in libvpx which is used in the webp module. Not sure if that is enough reason to rename the macro, instead of working around it locally..
Even though its unrelated, but since you are asking: I am getting overflow detection failures now using gcc. Its a little bit unfortunate that there is no default way to handle this. I would propose to use 128bit mult when available and fall back to "manual" multiplication otherwise. Unless someone has an easy way to fix this. Any ideas?
arne
On Sun, 30 Dec 2012, Jonas Walld�n @ Pike developers forum wrote:
Pushed now. If anything breaks please let me know.
This cleanup was kickstarted when I tried to compile 7.9 with Clang. I still see wrong execution in e.g. integer/bignum handling so I hope to have more patches in the coming days. One of my other findings was the commit aabfb4f04c5 (precompile.pike) which probably is a candidate for 7.8 backport as well but I'll let Grubba weigh in on that.
Too bad with the name clash, but a Pike-prefixed name would also be quite annoying due to its length.
Do you mean that my latest changes introduced the gcc overflow issues or only that you noticed them as of now? I'm also concerned about the overflow detection method using costly integer division on every Pike variable adjustment (machine-code excepted) so if we can optimize that it would be great. Here's a link that I found that could be useful:
https://www.securecoding.cert.org/confluence/display/seccode/INT32-C.+Ensure...
On Sun, 30 Dec 2012, Jonas Walld�n @ Pike developers forum wrote:
Too bad with the name clash, but a Pike-prefixed name would also be quite annoying due to its length.
Yes I agree. I will add an undef before the libvpx includes, which should fix that.
Do you mean that my latest changes introduced the gcc overflow issues or only that you noticed them as of now? I'm also concerned about the overflow detection method using costly integer division on every Pike variable adjustment (machine-code excepted) so if we can optimize that it would be great. Here's a link that I found that could be useful:
https://www.securecoding.cert.org/confluence/display/seccode/INT32-C.+Ensure...
As far as I can see its the latest change, which turned the macro into fuctions. I think its overall quite mysterious under which circumstances gcc (or clang) optimize that check away. In particular because compilers change and something that works now might break tomorrow. That said, I think it makes sense to use something which does _not_ involve any undefined behavior. Thanks for the link. I will have a look at that. I was thinking about using __int128 for those compilers that support it and "manual" multiplication otherwise (see http://www.fefe.de/intof.html).
arne
As far as I can see its the latest change, which turned the macro into fuctions. I think its overall quite mysterious under which circumstances gcc (or clang) optimize that check away. In particular because compilers change and something that works now might break tomorrow.
I don't know for sure if the old code relied on undefined behavior or if Clang simply miscompiled it. The macro -> function transformation did not really alter the semantics other than introducing volatile attributes so if the new form breaks gcc I suppose we were just lucky with the old one.
That said, I think it makes sense to use something which does _not_ involve any undefined behavior.
Agreed. If people mind the 7.9 instability with gcc I can revert the bignum patch since Clang isn't default even on OS X 10.8, but maybe we can give it a few days first.
On Mon, 31 Dec 2012, Jonas Walld�n @ Pike developers forum wrote:
As far as I can see its the latest change, which turned the macro into fuctions. I think its overall quite mysterious under which circumstances gcc (or clang) optimize that check away. In particular because compilers change and something that works now might break tomorrow.
I don't know for sure if the old code relied on undefined behavior or if Clang simply miscompiled it. The macro -> function transformation did not really alter the semantics other than introducing volatile attributes so if the new form breaks gcc I suppose we were just lucky with the old one.
Yes, I think so, too. The failures happen in the pike parser, where those overflow checks are optimized away and overflowing char constants are not detected.
That said, I think it makes sense to use something which does _not_ involve any undefined behavior.
Agreed. If people mind the 7.9 instability with gcc I can revert the bignum patch since Clang isn't default even on OS X 10.8, but maybe we can give it a few days first.
I dont think its an issue if 7.9 is temporarily broken. I started putting together a patch with standard compliant overflow checks based on the cert.org link you sent and some other sources. will push a branch once I have something presentable.
It's actually not a bad idea. Mind pushing the macros here? I think I could use them for my own projects. :)
Right. I based it off the last answer on this page, though the Pike version uses PIKE_CONCAT instead of ## and I skipped the C++ variant:
http://stackoverflow.com/questions/8776810/parameter-name-omitted-c-vs-c
pike-devel@lists.lysator.liu.se
-
Arne Goedeke -
Jonas Walld�n ＠ Pike developers forum -
Marcus Comstedt (ACROSS) (Hail Ilpalazzo!) ＠ Pike (-) developers forum
-
Martin Nilsson (Opera Mini - AFK!) ＠ Pike (-) developers forum
-
Mirar ＠ Pike developers forum
-
Peter Bortas ＠ Pike developers forum