Created attachment 13802 [details] Use `abort ()' instead of `assert (0)' Hi. A recent SME patch set for AArch64 that touches several opcodes/aarch64-* files uses the following pattern (with variations) in many places: int output; switch (input) { case /* ... */: output = /* ... */; break; default: assert (0); } action (output); Unless assert expands (i.e. is enabled) to something non-returning (or unreachable) at least GCC11 diagnoses output as potentially uninitialized variable at call site (and rightfully so) leading to a compile error. So, `assert (0)' should be replaced by something else that is always expanded. Attaching a patch that simply replaces all `assert (0)' with `abort ()'. This includes generated code. Given that `abort ()' ``results in the abnormal termination of the process'' the patch also removes all preceding assertation that always fire -- This and how unreachable code should be treated is of course up to debate, so the patch is simply a suggestion (but it does compile). Exluding gdb there are very few locations that use `assert (0)'. Gold uses gold_unreachable and gnulib/import/verify.h does not appear to provide any public interface that would provide a system agnostic __builtin_unreachable. I presume the latter would generate more optimal code than abort.
Comment on attachment 13802 [details] Use `abort ()' instead of `assert (0)' Oops.. opcodes/arm-dis.c shouldn't appear on the patch -- At least not with ``abort (); break;''!
The master branch has been updated by Nick Clifton <nickc@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=7060c28edd16a871fe1f8edaa8e49083a39b7ee5 commit 7060c28edd16a871fe1f8edaa8e49083a39b7ee5 Author: Nick Clifton <nickc@redhat.com> Date: Thu Nov 25 13:11:25 2021 +0000 Fix building the AArch64 assembler and disassembler when assertions are disabled. PR 28614 * aarch64-asm.c: Replace assert(0) with real code. * aarch64-dis.c: Likewise. * aarch64-opc.c: Likewise.
I am not a fan of using abort() inside a library. In my opinion library calls should never fail. If they cannot complete they should return some kind of error indication and allow their caller to decide what should be done. To that end I have applied my own patch which replaces the assert(0) invocations with "return false" or something similar.
Agreed, that is indeed a better option. Thanks!
(In reply to Nick Clifton from comment #3) > I am not a fan of using abort() inside a library. In my opinion library > calls should never fail. If they cannot complete they should return some > kind of error indication and allow their caller to decide what should be > done. > > To that end I have applied my own patch which replaces the assert(0) > invocations with "return false" or something similar. Perhaps we should turn off asserts in opcodes? to prevent similar usages in the future?
Hi Tamar, > https://sourceware.org/bugzilla/show_bug.cgi?id=28614 > > Perhaps we should turn off asserts in opcodes? to prevent similar usages in the > future? Well not just opcodes, but also the bfd library and libctf, and libiberty and ... What I have actually done is add a new build to my daily-test-the-binutils set of builds, this one configured as: configure --quiet --enable-targets-all --enable-64-bit-bfd CFLAGS="-DNDEBUG=1 -O2 -g" CXXFLAGS="-DNDEBUG=1 -O2 -g" Which I hope will mean that I will capture any future issues like this... Cheers Nick
(In reply to Nick Clifton from comment #6) > Hi Tamar, > > > https://sourceware.org/bugzilla/show_bug.cgi?id=28614 > > > > Perhaps we should turn off asserts in opcodes? to prevent similar usages in the > > future? > > Well not just opcodes, but also the bfd library and libctf, and libiberty > and ... > > What I have actually done is add a new build to my daily-test-the-binutils > set of builds, this one configured as: > > configure --quiet --enable-targets-all --enable-64-bit-bfd > CFLAGS="-DNDEBUG=1 -O2 -g" CXXFLAGS="-DNDEBUG=1 -O2 -g" > Ah great, I'll add these to mine as well. Cheers