Avoid undefined behaviour in m68hc11 md_begin

Avoid undefined behaviour in m68hc11 md_begin

[Alan Modra]

Given p = A where p is a pointer to some type and A is an array of
that type, then the expression p - 1 + 1 evokes undefined behaviour
according to the C standard.

gcc-13 -fsanitize=address,undefined complains about this, but not
where the undefined behaviour actually occurs at tc-m68hc11.c:646.
Instead you get an error: "tc-m68hc11.c:708:20: runtime error: store
to address 0x62600000016c with insufficient space for an object of
type 'int'".  Which is a lie.  There most definitely is space there.
Oh well, diagnostics are sometimes hard to get right.  The UB is easy
to avoid.

	PR 30279
	* config/tc-m68hc11.c (md_begin): Avoid undefined pointer
	decrement.  Remove unnecessary cast.

diff --git a/gas/config/tc-m68hc11.c b/gas/config/tc-m68hc11.c
index 7438e0dd51d..270ddf999ce 100644
--- a/gas/config/tc-m68hc11.c
+++ b/gas/config/tc-m68hc11.c
@@ -643,7 +643,7 @@ md_begin (void)
          (int (*) (const void*, const void*)) cmp_opcode);
 
   opc = XNEWVEC (struct m68hc11_opcode_def, num_opcodes);
-  m68hc11_opcode_defs = opc--;
+  m68hc11_opcode_defs = opc;
 
   /* Insert unique names into hash table.  The M6811 instruction set
      has several identical opcode names that have different opcodes based
@@ -655,19 +655,18 @@ md_begin (void)
 
       if (strcmp (prev_name, opcodes->name))
 	{
-	  prev_name = (char *) opcodes->name;
-
+	  prev_name = opcodes->name;
 	  opc++;
-	  opc->format = 0;
-	  opc->min_operands = 100;
-	  opc->max_operands = 0;
-	  opc->nb_modes = 0;
-	  opc->opcode = opcodes;
-	  opc->used = 0;
-	  str_hash_insert (m68hc11_hash, opcodes->name, opc, 0);
+	  (opc - 1)->format = 0;
+	  (opc - 1)->min_operands = 100;
+	  (opc - 1)->max_operands = 0;
+	  (opc - 1)->nb_modes = 0;
+	  (opc - 1)->opcode = opcodes;
+	  (opc - 1)->used = 0;
+	  str_hash_insert (m68hc11_hash, opcodes->name, opc - 1, 0);
 	}
-      opc->nb_modes++;
-      opc->format |= opcodes->format;
+      (opc - 1)->nb_modes++;
+      (opc - 1)->format |= opcodes->format;
 
       /* See how many operands this opcode needs.  */
       expect = 0;
@@ -700,14 +699,13 @@ md_begin (void)
 	    expect++;
 	}
 
-      if (expect < opc->min_operands)
-	opc->min_operands = expect;
+      if (expect < (opc - 1)->min_operands)
+	(opc - 1)->min_operands = expect;
       if (IS_CALL_SYMBOL (opcodes->format))
 	expect++;
-      if (expect > opc->max_operands)
-	opc->max_operands = expect;
+      if (expect > (opc - 1)->max_operands)
+	(opc - 1)->max_operands = expect;
     }
-  opc++;
   m68hc11_nb_opcode_defs = opc - m68hc11_opcode_defs;
 
   if (flag_print_opcodes)

-- 
Alan Modra
Australia Development Lab, IBM

Re: Avoid undefined behaviour in m68hc11 md_begin

[Andreas Schwab]

On Mär 28 2023, Alan Modra via Binutils wrote:

> Given p = A where p is a pointer to some type and A is an array of
> that type, then the expression p - 1 + 1 evokes undefined behaviour
> according to the C standard.
>
> gcc-13 -fsanitize=address,undefined complains about this, but not
> where the undefined behaviour actually occurs at tc-m68hc11.c:646.
> Instead you get an error: "tc-m68hc11.c:708:20: runtime error: store
> to address 0x62600000016c with insufficient space for an object of
> type 'int'".  Which is a lie.

It's not a lie if you regard p-1 as no longer being associated with A,
so that p-1+1 cannot move back into it any more.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Avoid undefined behaviour in m68hc11 md_begin

[Alan Modra]

On Tue, Mar 28, 2023 at 01:19:38PM +0200, Andreas Schwab wrote:
> On Mär 28 2023, Alan Modra via Binutils wrote:
> 
> > Given p = A where p is a pointer to some type and A is an array of
> > that type, then the expression p - 1 + 1 evokes undefined behaviour
> > according to the C standard.
> >
> > gcc-13 -fsanitize=address,undefined complains about this, but not
> > where the undefined behaviour actually occurs at tc-m68hc11.c:646.
> > Instead you get an error: "tc-m68hc11.c:708:20: runtime error: store
> > to address 0x62600000016c with insufficient space for an object of
> > type 'int'".  Which is a lie.
> 
> It's not a lie if you regard p-1 as no longer being associated with A,
> so that p-1+1 cannot move back into it any more.

The error message specifies an address.  That address is part of the
memory allocated.  To say there is insufficient space at that address
is indeed a lie.

Please note that I'm not saying anyone should spend time fixing this
diagnostic.  I truly don't care.  After all, it was good enough to
find the undefined behaviour in a relatively short time.

-- 
Alan Modra
Australia Development Lab, IBM

Re: Avoid undefined behaviour in m68hc11 md_begin

[Andreas Schwab]

On Mär 28 2023, Alan Modra wrote:

> On Tue, Mar 28, 2023 at 01:19:38PM +0200, Andreas Schwab wrote:
>> On Mär 28 2023, Alan Modra via Binutils wrote:
>> 
>> > Given p = A where p is a pointer to some type and A is an array of
>> > that type, then the expression p - 1 + 1 evokes undefined behaviour
>> > according to the C standard.
>> >
>> > gcc-13 -fsanitize=address,undefined complains about this, but not
>> > where the undefined behaviour actually occurs at tc-m68hc11.c:646.
>> > Instead you get an error: "tc-m68hc11.c:708:20: runtime error: store
>> > to address 0x62600000016c with insufficient space for an object of
>> > type 'int'".  Which is a lie.
>> 
>> It's not a lie if you regard p-1 as no longer being associated with A,
>> so that p-1+1 cannot move back into it any more.
>
> The error message specifies an address.  That address is part of the
> memory allocated.  To say there is insufficient space at that address
> is indeed a lie.

It's the same lie when you fall off the end of an array into a
neighboring array.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Avoid undefined behaviour in m68hc11 md_begin

[Alan Modra]

On Tue, Mar 28, 2023 at 02:30:10PM +0200, Andreas Schwab wrote:
> On Mär 28 2023, Alan Modra wrote:
> 
> > On Tue, Mar 28, 2023 at 01:19:38PM +0200, Andreas Schwab wrote:
> >> On Mär 28 2023, Alan Modra via Binutils wrote:
> >> 
> >> > Given p = A where p is a pointer to some type and A is an array of
> >> > that type, then the expression p - 1 + 1 evokes undefined behaviour
> >> > according to the C standard.
> >> >
> >> > gcc-13 -fsanitize=address,undefined complains about this, but not
> >> > where the undefined behaviour actually occurs at tc-m68hc11.c:646.
> >> > Instead you get an error: "tc-m68hc11.c:708:20: runtime error: store
> >> > to address 0x62600000016c with insufficient space for an object of
> >> > type 'int'".  Which is a lie.
> >> 
> >> It's not a lie if you regard p-1 as no longer being associated with A,
> >> so that p-1+1 cannot move back into it any more.
> >
> > The error message specifies an address.  That address is part of the
> > memory allocated.  To say there is insufficient space at that address
> > is indeed a lie.
> 
> It's the same lie when you fall off the end of an array into a
> neighboring array.

I'm beginning to think you are deliberately provoking me.  There is no
falling off the end of an array here.  See the testcase in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109308.

-- 
Alan Modra
Australia Development Lab, IBM

Re: Avoid undefined behaviour in m68hc11 md_begin

[Andreas Schwab]

On Mär 29 2023, Alan Modra wrote:

> On Tue, Mar 28, 2023 at 02:30:10PM +0200, Andreas Schwab wrote:
>> On Mär 28 2023, Alan Modra wrote:
>> 
>> > On Tue, Mar 28, 2023 at 01:19:38PM +0200, Andreas Schwab wrote:
>> >> On Mär 28 2023, Alan Modra via Binutils wrote:
>> >> 
>> >> > Given p = A where p is a pointer to some type and A is an array of
>> >> > that type, then the expression p - 1 + 1 evokes undefined behaviour
>> >> > according to the C standard.
>> >> >
>> >> > gcc-13 -fsanitize=address,undefined complains about this, but not
>> >> > where the undefined behaviour actually occurs at tc-m68hc11.c:646.
>> >> > Instead you get an error: "tc-m68hc11.c:708:20: runtime error: store
>> >> > to address 0x62600000016c with insufficient space for an object of
>> >> > type 'int'".  Which is a lie.
>> >> 
>> >> It's not a lie if you regard p-1 as no longer being associated with A,
>> >> so that p-1+1 cannot move back into it any more.
>> >
>> > The error message specifies an address.  That address is part of the
>> > memory allocated.  To say there is insufficient space at that address
>> > is indeed a lie.
>> 
>> It's the same lie when you fall off the end of an array into a
>> neighboring array.
>
> I'm beginning to think you are deliberately provoking me.  There is no
> falling off the end of an array here.

I didn't say that.  I was just comparing it to other situations where
there is insufficient space the address even though the memory exists.
Similar to this there is no allocated space at the address p-1, so p-1+1
cannot be part of an allocated space.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."