Bug 58192

Summary: G++ emits incorrect code when passing enum classes as function parameters
Product: gcc Reporter: Kenton Varda <temporal>
Component: c++Assignee: Not yet assigned to anyone <unassigned>
Status: RESOLVED DUPLICATE    
Severity: normal CC: hjl.tools, jakub, ppluzhnikov, rth, ubizjak
Priority: P2 Keywords: wrong-code
Version: 4.8.1   
Target Milestone: ---   
Host: Target:
Build: Known to work:
Known to fail: Last reconfirmed: 2013-08-19 00:00:00
Attachments: Demonstration of enum class passing bug
Demonstration of enum class passing bug (v2)

Description Kenton Varda 2013-08-19 06:27:17 UTC
Demonstration attached.  It repros with g++ 4.8.1 (tested on Ubuntu and Cygwin).  I think I saw similar problems with g++ 4.7.3, but this particular test case does not seem to repro with it.
Comment 1 Kenton Varda 2013-08-19 06:29:32 UTC
Created attachment 30672 [details]
Demonstration of enum class passing bug
Comment 2 Kenton Varda 2013-08-19 06:38:03 UTC
Note:  Both the Ubuntu and Cygwin systems were x86_64.  I don't know if the problem occurs in 32-bit mode.
Comment 3 Kenton Varda 2013-08-19 16:48:58 UTC
Created attachment 30673 [details]
Demonstration of enum class passing bug (v2)

The first version of my demo was doing slightly weird things with unions in main().  This was vestigial code from my attempts to narrow down the bug, but wasn't actually necessary to trigger it.  So, I simplified it.
Comment 4 Paolo Carlini 2013-08-19 16:59:09 UTC
I have yet to analyze this in detail, but let's play safe and confirm it as wrong-code.
Comment 5 Jakub Jelinek 2013-08-19 18:09:14 UTC
The difference is that in the S1::<anon struct>::set(Foo) case
setup_incoming_promotions in combine.c says that the argument has non-zero mask of 0xff, because the function is local to the TU, while for S2 all caller's aren't guaranteed local and thus it isn't optimized.  The argument type is Foo (QImode) passed in SImode register, guess combine.c expects that caller zero-extends in this case, but it doesn't (in neither case).

enum class Foo : unsigned char { FOO };
struct S1
{
  struct
  {
    unsigned int i;
    void __attribute__((noinline))
    set (Foo foo)
    {
      i = static_cast<unsigned int> (foo);
    }
  } x;
};
struct S2
{
  struct X
  {
    unsigned int i;
    void __attribute__((noinline))
    set (Foo foo)
    {
      i = static_cast<unsigned int> (foo);
    }
  };
  X x;
};
struct T { unsigned short i; Foo f; unsigned char c; };

__attribute__((noinline)) void
baz (unsigned int x)
{
  if (x != 0) __builtin_abort ();
}

void
bar (T t)
{
  S1 s1;
  s1.x.set(t.f);
  baz (s1.x.i);
  S2 s2;
  s2.x.set(t.f);
  baz (s2.x.i);
}

int
main()
{
  T t = { 0xabcd, Foo::FOO, 0xef};
  bar (t);
}

For:
unsigned int v1, v2;

__attribute__((noinline, noclone)) static void
foo (unsigned char a)
{
  v1 = a;
}

__attribute__((noinline, noclone)) void
bar (unsigned char a)
{
  v2 = a;
}

void
baz (unsigned int a)
{
  foo (a);
  bar (a);
}
the situation is similar for foo/bar (foo optimizes and doesn't zero extend from 8 to 32 bits, bar doesn't), but the caller in this case always zero extends.
Comment 6 Paolo Carlini 2014-09-18 09:20:20 UTC
Jakub, do I understand correctly that this isn't a front-end issue?
Comment 7 Jakub Jelinek 2014-09-18 10:01:31 UTC
That is to be determined.  Either it might be an x86_64 bug in passing such types, or FE issue, middle-end.
Comment 8 Uroš Bizjak 2014-09-19 12:10:06 UTC
(In reply to Jakub Jelinek from comment #7)
> That is to be determined.  Either it might be an x86_64 bug in passing such
> types, or FE issue, middle-end.

Please note that the error also occurs with -m32, so it seems highly unlikely an x86_64 target-dependent bug.
Comment 9 Uroš Bizjak 2014-09-19 13:06:58 UTC
Looking at dumps, obtained with -m32 -O1, we have following sequence before the call to _ZN2S1Ut_3setE3Foo:

_.dfinit:

(insn 7 3 8 2 (parallel [
            (set (reg:SI 88)
                (lshiftrt:SI (reg/v:SI 86 [ t ])
                    (const_int 16 [0x10])))
            (clobber (reg:CC 17 flags))
        ]) 539 {*lshrsi3_1}
     (nil))
(insn 8 7 9 2 (parallel [
            (set (reg:QI 85 [ t$2 ])
                (and:QI (subreg:QI (reg:SI 88) 0)
                    (const_int -1 [0xffffffffffffffff])))
            (clobber (reg:CC 17 flags))
        ]) 379 {*andqi_1}
     (nil))

...

(insn 10 9 11 2 (set (reg:QI 1 dx)
        (reg:QI 85 [ t$2 ])) gcc-bug.cc:72 93 {*movqi_internal}
     (nil))

...

(call_insn 12 11 13 2 (call (mem:QI (symbol_ref:SI ("_ZN2S1Ut_3setE3Foo") [flags 0x3]  <function_decl 0x2ad94c491438 set>) [0 set S1 A8])
        (const_int 0 [0])) gcc-bug.cc:72 651 {*call}
     (expr_list:REG_EH_REGION (const_int 0 [0])
        (nil))
    (expr_list:SI (use (reg:SI 0 ax))
        (expr_list:QI (use (reg:QI 1 dx))
            (nil))))

_.cse1 mangles argument setup to:

(insn 8 7 9 2 (set (reg:QI 85 [ t$2 ])
        (subreg:QI (reg:SI 88) 0)) 93 {*movqi_internal}
     (expr_list:REG_DEAD (reg:SI 88)
        (expr_list:REG_UNUSED (reg:CC 17 flags)
            (nil))))

...

(insn 10 9 11 2 (set (reg:QI 1 dx)
        (subreg:QI (reg:SI 88) 0)) gcc-bug.cc:72 93 {*movqi_internal}
     (nil))

and _.fwprop1 merges both to:

(insn 10 9 11 2 (set (reg:QI 1 dx)
        (subreg:QI (reg:SI 88) 0)) gcc-bug.cc:72 93 {*movqi_internal}
     (nil))

resulting in:

#(insn 10 9 34 2 (set (reg:QI 1 dx)
#        (reg:QI 3 bx [88])) gcc-bug.cc:72 93 {*movqi_internal}
#     (nil))
	movl	%ebx, %edx	# 10	*movqi_internal/1	[length = 2]

However, x86 ABI mandates that arguments should be properly extended in the call to the function. I don't think original (insn 8) is correct, as it is effectively a NOP.
Comment 10 Jakub Jelinek 2014-09-19 14:10:33 UTC
When I compile:
#ifdef CHAR
typedef unsigned char Foo;
#else
enum class Foo : unsigned char { FOO };
#endif
unsigned int v1, v2;

__attribute__((noinline, noclone)) static void
foo (Foo a)
{
  v1 = (unsigned int) a;
}

__attribute__((noinline, noclone)) void
bar (Foo a)
{
  v2 = (unsigned int) a;
}

void
baz (unsigned int a)
{
  foo ((Foo) a);
  bar ((Foo) a);
}

with -DCHAR vs. -UCHAR, there is a difference visible already in *.original dump:
 <<cleanup_point <<< Unknown tree: expr_stmt
-  foo ((int) (Foo) a) >>>>>;
+  foo ((Foo) a) >>>>>;
 <<cleanup_point <<< Unknown tree: expr_stmt
-  bar ((int) (Foo) a) >>>>>;
+  bar ((Foo) a) >>>>>;
i.e. there is explicit zero-extension for integral types smaller than int in the IL (i.e. for the -DCHAR) case, but nothing like that for the enum class with underlying type smaller than int.  And presumably the middle-end and backends rely on this.
Comment 11 Uroš Bizjak 2014-12-15 14:30:00 UTC
Does this still fail after r218720, AKA PR64037 [1]?

[1] https://gcc.gnu.org/ml/gcc-patches/2014-12/msg01183.html
Comment 12 H.J. Lu 2014-12-15 14:55:15 UTC
(In reply to Uroš Bizjak from comment #11)
> Does this still fail after r218720, AKA  [1]?
> 
> [1] https://gcc.gnu.org/ml/gcc-patches/2014-12/msg01183.html

I think it is a dup of PR64037.
Comment 13 H.J. Lu 2014-12-15 16:01:43 UTC
Dup of PR 64037.  Will be fixed after 4.8.4 is released.

*** This bug has been marked as a duplicate of bug 64037 ***