Bug 28614 - [AArch64] opcodes/aarch64-*: -Werror=maybe-uninitialized hit unless assertations are enabled
Summary: [AArch64] opcodes/aarch64-*: -Werror=maybe-uninitialized hit unless assertati...
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: binutils (show other bugs)
Version: 2.38
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-11-22 13:25 UTC by Pekka Seppänen
Modified: 2021-11-26 11:53 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
Use `abort ()' instead of `assert (0)' (2.78 KB, patch)
2021-11-22 13:25 UTC, Pekka Seppänen
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pekka Seppänen 2021-11-22 13:25:48 UTC
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 1 Pekka Seppänen 2021-11-22 13:29:50 UTC
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;''!
Comment 2 cvs-commit@gcc.gnu.org 2021-11-25 13:11:56 UTC
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.
Comment 3 Nick Clifton 2021-11-25 13:14:17 UTC
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.
Comment 4 Pekka Seppänen 2021-11-25 13:22:11 UTC
Agreed, that is indeed a better option.  Thanks!
Comment 5 Tamar Christina 2021-11-26 11:44:52 UTC
(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?
Comment 6 Nick Clifton 2021-11-26 11:51:39 UTC
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
Comment 7 Tamar Christina 2021-11-26 11:53:36 UTC
(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