Re: [PATCH 1/3] x86: work around compiler diagnosing dangling pointer

[Alan Modra <amodra@gmail.com>]

On Mon, Apr 24, 2023 at 09:34:27AM +0200, Jan Beulich via Binutils wrote:
> For quite come time print_insn() has been storing the address of a local
> variable into info->private_data. Since the compiler can't know that the
> field won't be accessed again after print_insn() returns, it may kind of
> legitimately diagnose this. And recent enough gcc does as of the
> introduction of the fetch_error() return paths (replacing setjmp()-based
> error handling).
> 
> Utilizing that neither prefix_name() nor i386_dis_printf() actually use
> info->private_data, zap the pointer in fetch_error(), after having
> retrieved it for local use.
> ---
> Let's hope that this addresses the observed issues, which I haven't been
> seeing myself. And of course there are further return paths which may
> (sooner or later) also have such a warning trigger.

I'll be surprised if your patch is enough.  I have the following in my
local tree, tested to work with a freshly built gcc-13 compiler.
Would you like me to commit this (and revert your patch)?

opcodes/i386-dis.c: In function ‘print_insn’:
opcodes/i386-dis.c:9865:22: error: storing the address of local
variable ‘priv’ in ‘*info.private_data’ [-Werror=dangling-pointer=]

	* i386-dis.c (print_insn): Clear info->private_data before
	returning.

diff --git a/opcodes/i386-dis.c b/opcodes/i386-dis.c
index f021bdaa3e7..01e5ba81723 100644
--- a/opcodes/i386-dis.c
+++ b/opcodes/i386-dis.c
@@ -9731,6 +9731,7 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
 {
   const struct dis386 *dp;
   int i;
+  int ret;
   char *op_txt[MAX_OPERANDS];
   int needcomma;
   bool intel_swap_2_3;
@@ -9887,16 +9888,21 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
 	i386_dis_printf (&ins, dis_style_mnemonic, "%s%s",
 			 (i == 0 ? "" : " "),
 			 prefix_name (&ins, ins.all_prefixes[i], sizeflag));
-      return i;
+      ret = i;
+      goto out;
 
     case ckp_fetch_error:
-      return fetch_error (&ins);
+      goto fetch_error_out;
     }
 
   ins.insn_codep = ins.codep;
 
   if (!fetch_code (info, ins.codep + 1))
-    return fetch_error (&ins);
+    {
+    fetch_error_out:
+      ret = fetch_error (&ins);
+      goto out;
+    }
 
   ins.two_source_ops = (*ins.codep == 0x62) || (*ins.codep == 0xc8);
 
@@ -9909,7 +9915,8 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
 	i386_dis_printf (&ins, dis_style_mnemonic, "%s ",
 			 prefix_name (&ins, ins.all_prefixes[i], sizeflag));
       i386_dis_printf (&ins, dis_style_mnemonic, "fwait");
-      return i + 1;
+      ret = i + 1;
+      goto out;
     }
 
   if (*ins.codep == 0x0f)
@@ -9918,7 +9925,7 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
 
       ins.codep++;
       if (!fetch_code (info, ins.codep + 1))
-	return fetch_error (&ins);
+	goto fetch_error_out;
       threebyte = *ins.codep;
       dp = &dis386_twobyte[threebyte];
       ins.need_modrm = twobyte_has_modrm[threebyte];
@@ -9942,30 +9949,30 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
 
   ins.end_codep = ins.codep;
   if (ins.need_modrm && !fetch_modrm (&ins))
-    return fetch_error (&ins);
+    goto fetch_error_out;
 
   if (dp->name == NULL && dp->op[0].bytemode == FLOATCODE)
     {
       if (!get_sib (&ins, sizeflag)
 	  || !dofloat (&ins, sizeflag))
-	return fetch_error (&ins);
+	goto fetch_error_out;
     }
   else
     {
       dp = get_valid_dis386 (dp, &ins);
       if (dp == &err_opcode)
-	return fetch_error (&ins);
+	goto fetch_error_out;
       if (dp != NULL && putop (&ins, dp->name, sizeflag) == 0)
 	{
 	  if (!get_sib (&ins, sizeflag))
-	    return fetch_error (&ins);
+	    goto fetch_error_out;
 	  for (i = 0; i < MAX_OPERANDS; ++i)
 	    {
 	      ins.obufp = ins.op_out[i];
 	      ins.op_ad = MAX_OPERANDS - 1 - i;
 	      if (dp->op[i].rtn
 		  && !dp->op[i].rtn (&ins, dp->op[i].bytemode, sizeflag))
-		return fetch_error (&ins);
+		goto fetch_error_out;
 	      /* For EVEX instruction after the last operand masking
 		 should be printed.  */
 	      if (i == 0 && ins.vex.evex)
@@ -10055,14 +10062,16 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
   if (ins.need_vex && ins.vex.register_specifier != 0)
     {
       i386_dis_printf (&ins, dis_style_text, "(bad)");
-      return ins.end_codep - priv.the_buffer;
+      ret = ins.end_codep - priv.the_buffer;
+      goto out;
     }
 
   /* If EVEX.z is set, there must be an actual mask register in use.  */
   if (ins.vex.zeroing && ins.vex.mask_register_specifier == 0)
     {
       i386_dis_printf (&ins, dis_style_text, "(bad)");
-      return ins.end_codep - priv.the_buffer;
+      ret = ins.end_codep - priv.the_buffer;
+      goto out;
     }
 
   switch (dp->prefix_requirement)
@@ -10073,7 +10082,8 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
       if (ins.need_vex ? !ins.vex.prefix : !(ins.prefixes & PREFIX_DATA))
 	{
 	  i386_dis_printf (&ins, dis_style_text, "(bad)");
-	  return ins.end_codep - priv.the_buffer;
+	  ret = ins.end_codep - priv.the_buffer;
+	  goto out;
 	}
       ins.used_prefixes |= PREFIX_DATA;
       /* Fall through.  */
@@ -10100,7 +10110,8 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
 	      && !ins.vex.w != !(ins.used_prefixes & PREFIX_DATA)))
 	{
 	  i386_dis_printf (&ins, dis_style_text, "(bad)");
-	  return ins.end_codep - priv.the_buffer;
+	  ret = ins.end_codep - priv.the_buffer;
+	  goto out;
 	}
       break;
 
@@ -10156,7 +10167,8 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
   if ((ins.codep - ins.start_codep) > MAX_CODE_LENGTH)
     {
       i386_dis_printf (&ins, dis_style_text, "(bad)");
-      return MAX_CODE_LENGTH;
+      ret = MAX_CODE_LENGTH;
+      goto out;
     }
 
   /* Calculate the number of operands this instruction has.  */
@@ -10264,7 +10276,10 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
 	  info);
 	break;
       }
-  return ins.codep - priv.the_buffer;
+  ret = ins.codep - priv.the_buffer;
+ out:
+  info->private_data = NULL;
+  return ret;
 }
 
 /* Here for backwards compatibility.  When gdb stops using

-- 
Alan Modra
Australia Development Lab, IBM