Bug 21059 - Bogus warning about clobbered variable
Summary: Bogus warning about clobbered variable
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.0.0
: P2 normal
Target Milestone: 4.3.0
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
Depends on:
Blocks:
 
Reported: 2005-04-16 16:56 UTC by Andreas Schwab
Modified: 2009-02-20 22:42 UTC (History)
3 users (show)

See Also:
Host:
Target: ia64-*-*
Build:
Known to work: 4.3.0 4.3.1 4.3.2 4.4.0
Known to fail: 4.0.2 4.0.3 4.0.4 4.1.0 4.1.1 4.1.2 4.2.0 4.2.1 4.2.2 4.2.3 4.2.4
Last reconfirmed: 2005-11-26 07:39:13


Attachments
Delay auto-inc generation until after determining which regs live across setjmp. (1.75 KB, patch)
2005-12-02 04:12 UTC, Jim Wilson
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Schwab 2005-04-16 16:56:39 UTC
$ cat m68k-dis.i
typedef int (*fprintf_ftype) (void *, const char*, ...);
typedef struct disassemble_info {
  fprintf_ftype fprintf_func;
  void *stream;
  int (*read_memory_func) (void);
} disassemble_info;
extern char *foo (void);
typedef struct { } jmp_buf[1];
extern int setjmp (jmp_buf);
int
print_insn_m68k (disassemble_info *info)
{
  jmp_buf bailout;
  if (setjmp (bailout) != 0)
    return -1;
  info->fprintf_func (info->stream, foo ());
  return 0;
}
$ gcc/xgcc -B gcc/ -W -Wall -O2 -S m68k-dis.i -v
Reading specs from gcc/specs
Target: ia64-suse-linux
Configured with: ../configure --host=ia64-suse-linux --enable-shared --enable-threads --enable-__cxa_atexit --with-system-zlib --with-system-libunwind
Thread model: posix
gcc version 4.0.0 20050416 (prerelease)
 gcc/cc1 -fpreprocessed m68k-dis.i -quiet -dumpbase m68k-dis.i -auxbase m68k-dis -O2 -W -Wall -version -o m68k-dis.s
GNU C version 4.0.0 20050416 (prerelease) (ia64-suse-linux)
        compiled by GNU C version 4.0.0 20050416 (prerelease).
GGC heuristics: --param ggc-min-expand=100 --param ggc-min-heapsize=131072
m68k-dis.i: In function �פערprint_insn_m68k�פעש:
m68k-dis.i:11: warning: argument �פערinfo�פעש might be clobbered by �פערlongjmp�פעש or �פערvfork�פעש
Comment 1 Steve Ellcey 2005-06-01 22:08:29 UTC
It is not clear to me if this bug is about whether or not we should put out the
message or if it is about the format of the message,  I.e. that the quotes are
messed up.
Comment 2 Andreas Schwab 2005-06-01 23:03:01 UTC
The warning is just wrong.  There is nothing that can be clobbered by longjmp. 
Comment 3 Tom Truscott 2005-06-02 18:40:21 UTC
The current warning algorithm is too simple.  This would be better: 

   For each function that contains call(s) to setjmp(), compute:

     ref_nz The set of variables that might possibly be live
             (referenced) after a setjmp() returns a non-zero value.
     set_any The set of variables that might possibly be set
             (defined) after a call to setjmp() returns.

   Issue a warning for all variables in the intersection of the sets.
Comment 4 Jim Wilson 2005-12-02 02:24:35 UTC
The spurious warning is a problem because binutils is compiled by -Werror by default now, when compiled by gcc.  This spurious warning thus causes a build failure for binutils.  This build failure does not show up in a native ia64-linux build fortunately, it only shows up for cross builds, or if --enable-targets=all is used.  Still, this means it is a problem for people trying to do binutils development on an ia64-linux host.  This may also cause problems with other programs as use of -Werror spreads.

Also, the spurious warning is a problem because the warning is inherently unportable.  Different targets and/or different optimization options can result in different sets of warnings.  So what happens is that one person compiles the code and gets no warning.  Another compiles the code for a different target, gets a warning, and submits a patch.  Then the first person points out that the code isn't broken, that the patch shouldn't necessary, and recommends that the compiler should be fixed.  Hence this PR.

The failure happens for ia64 because of the lack of reg+offset addressing modes.  We have a parameter info, copied into pseudo-reg 345, that is dereferenced twice, once for the fprintf_func field and once for the stream field.  These fields are at offset 0 and 4.  Normally, we would get two mems one with address (reg 345) and one with address (plus (reg 345) (const_int 4)).  But IA-64 does not have reg+offset, so we end up with a post-increment of reg 345.  This means reg 345 is now set twice.  Once before the setjmp, and once afterwards.  This triggers the code in regno_clobbered_at_setjmp which has a test "REG_N_SETS (regno) > 1" which is now true for IA-64, but not for most other targets.

The best fix I see is to improve the CFG to include info about which block after setjmp is the fallthrough and which is the return from longjmp path.  Once we have the improved CFG, then we can implement a more accurate test, such as the one tft mentioned in comment #3.  It may be easier and/or more appropriate to do this work in the tree-ssa part of the compiler.  This is likely more work than I can do at the moment for this PR.

A more limited solution may be possible by adjusting the setjmp warning code to take these post-inc sets into account.  We could keep track of sets created by auto-inc optimizations, and then subtract them in the setjmp warning code.  This would ensure the same results for all targets, regardless of addressing modes.  The flaw here is that if auto-inc opts do modify a register, then we really do have a problem, and really should have emitted a warning.  So this doesn't seem worth pursuing.

Alternatively, we could disable auto-inc optimization for pseudo regs that live across setjmp.  Such pseudo regs won't be allocated to hard regs, so use of auto-inc addressing modes isn't profitable here anyways.  I think this is a much better idea.  Unfortunately, it can't be easily implemented, as we can perform auto-inc optimizations before regs_live_at_setjmp is set.  We would need another  update_life_info call to do this.  Not clear if this is worthwhile.  I think it is worth investigating though.

Another alternative is to just remove the warning code.  Most warnings emitted by this code are probably spurious.  This may not be a viable solution though, since the warnings are actually useful on some occasions, and the lack of warnings for problematic code could be considered a regression.  This is probably not feasible.
Comment 5 Jim Wilson 2005-12-02 04:12:18 UTC
Created attachment 10387 [details]
Delay auto-inc generation until after determining which regs live across setjmp.

This is my proposed untested patch.

While writing this, I realized that we have an actual optimization bug here, in addition to the spurious warning problem.  ISO C says that if a variable is not set in between setjmp and longjmp, then its value is preserved by the longjmp.  GCC implements this by forcing all such variables into stack slots.  Now suppose we have code like this, where reg100 is a user variable.
  (set reg100 ...)
  (call setjmp)
  ... (mem reg100) ...
  (set (reg101) (plus (reg100) (const_int 8)))
  ... (mem reg101) ...
  (call longjmp)
The value of reg100 should be unchanged.  On IA-64 however, gcc converts this code to
  (set reg100 ...)
  (call setjmp)
  ... (mem (post_inc (reg100) (const_int 8)) ...
  ... (mem reg101) ...
  (call longjmp)
and now the value of reg100 is being modified, which violates the ISO C spec.  If program execution can return to the mems after the longjmp, then the code will not function correctly.  The wrong values will be read from memory.  This could be a very hard bug to diagnose if it occured in real programs, and if the longjmp call was in an exception path that rarely occured.

There is also an addition problem here that performing auto-inc optimization on variables that are forced to stack slots is a de-optimization, as now we need an additional memory store that we did not need before.  Without my patch, I get
        ld8 r14 = [r15]
        ;;
        adds r14 = 8, r14
        ;;
        st8 [r15] = r14
        adds r14 = -8, r14
        ;;
        ld8 r34 = [r14], 8
        ;;
        ld8 r33 = [r14]
with my patch I get
        ld8 r15 = [r15]
        ;;
        ld8 r33 = [r15]
        adds r14 = 8, r15
        ;;
        ld8 r34 = [r14]
which is much better.  Not all cases are as lucky as this though.  In some cases the use of auto-inc addresses would have been better.  Since this only affects functions that call setjmp, it shouldn't be a serious problem.

One more note, I noticed that we are setting regs_live_at_setjmp, but never clearing it.  This means it could contain more registers than it should.  But since this only affects functions that call setjmp, I'm not going to worry about it.
Comment 6 Jim Wilson 2005-12-02 19:16:33 UTC
Oops.  That patch should have a "| PROP_AUTOINC" in the new update_life_info call.  That would explain why I was seeing some unexpected de-optimizations with the patch.
Comment 7 Steve Ellcey 2009-02-20 22:36:19 UTC
It looks like this was fixed in the 4.3 time frame so I am closing the defect.