| Message ID | 29C8CB44-A0EC-4A9A-A4D3-3ABB6D66DA5B@comcast.net |
|---|---|
| State | New |
| Headers | show |
On Tue, Nov 3, 2015 at 10:58 PM, Mike Stump <mikestump@comcast.net> wrote: > On Nov 3, 2015, at 12:46 AM, Richard Sandiford <richard.sandiford@arm.com> wrote: >> This isn't just an argument about the DWARF standard though. It's an >> argument about GCC internals. Presumably these hypothetical BLKmode >> types would need to support addition, > > I don’t recall seeing that as a requirement in the dwarf standard, nor in the language standard of the hypothetical language I posited. Further, I don’t recall seeing a prohibition on fetching from memory, combining three byte to form a 24 bit value in a 64 bit register on x86_64, nor the prohibition on doing a 64-bit add on that architecture. Do you have a quote to support your position? Further, I don’t even see support that add must be supported in the language front end I posited. Since I control it, you can’t can’t require much of anything in it. > >> but plus:BLK is not well formed, > > BLKmode is well formed in gcc. I can’t image what this even means. > >> and wouldn't distinguish between your 3-byte and 152-bit cases. > > A 3 byte value in dwarf has a byte length of 3, and a 152 bit value has a byte length of 19. I fail to see why in dwarf they cannot be distinguished. A cleaver front end can track the values in that front end, and regions of memory that is uses to back objects, usually the front end does track objects, and when asked to, will generate dwarf for them. I believe that front ends are able to track object byte sized for objects. I believe that the C++ front end does this for example, and will generate dwarf. > >> I don’t think const_int and const_wide_int are logically different. > > They aren’t. Indeed, the code for const_wide_int was copied from const_int and/or const_double and only enhanced to have an arbitrary size. > >> There's the >> historical decision that const_int doesn't have a stored mode, but I >> don't think that was because we wanted to support const_ints that are >> conceptually BLKmode. > > So, given that what must be, given the semantics of dwarf, and the totality of semantics available to a front end, BLKmode values are perfectly well defined. I don’t know why you would think otherwise. Further, arguing that we must break them, because… is silly, without the because elaborated. A constant value that is known at compile time to be an arbitrary value whose type is a signed n byte int can be represented in const_int when n fits, and can be expressed as a const_wide_int when n fits and can be expressed in dwarf. Further, const_int is not unreasonable to represent constants, further, there is no prohibition on a front end having constants, nor on the size of those constants being n bytes in size, at least for values of n where n can be supported directly by const_wide_int. If there were one, you could be able to quote it. > >> I think from an rtl perspective the only sensible way for frontends to >> cope with integers whose size doesn't match an rtl mode is to promote >> to the next widest mode, > > No, again, to support your position, you’d have to quote the part of gcc that disclaims support. You have not. You can think there is a prohibition on 3 byte ints, if you want. I can’t fix that. What I can object to, is adding a line to the document that says that gcc cannot be used to write a front end that supports 3 byte ints, or 19 byte ints. > >> which is what the stor-layout.c code I quoted >> does. Obviously if your 3 byte type is actually 3 bytes in memory rather >> than 4, and no 3-byte mode is available, you can't just load and store >> the value using a normal rtl move. You have to use bitfield extraction >> and insertion instead. > > So? Another technique that a front end could use is to fetch a HImode, and a QImode value, shift and or them together, but no matter. > >> I picked this PR up because it was wide-int-related, even though >> (as is probably all too obvious from this thread) I'm not familiar >> with the dwarf2out.c code. It's actually your commit that I'm trying >> to fix here (r201707). Would you mind taking the PR over and handling >> it the way you think it should be handled? > > So, I did up the patch: > > Index: rtl.h > =================================================================== > --- rtl.h (revision 229720) > +++ rtl.h (working copy) > @@ -2086,6 +2086,15 @@ > inline unsigned int > wi::int_traits <rtx_mode_t>::get_precision (const rtx_mode_t &x) > { > + /* VOIDmode CONST_WIDE_INT pairings are used for debug information > + when the type of the underlying object has not been tracked and > + is unimportant so we form a precision from the value of the > + constant. This is sufficient for dwarf generation as dwarf can > + track the type separately from the value. */ > + if (x.second == VOIDmode > + && CONST_WIDE_INT_P (x.first)) > + return CONST_WIDE_INT_NUNITS (x.first) * HOST_BITS_PER_WIDE_INT; > + > return GET_MODE_PRECISION (x.second); > } I think you should limit the effect of this patch to the dwarf2out use as the above doesn't make sense to me. As Richard said, a (plus:BLK ...) isn't well-formed. Ideally we'd have an assert that you don't create a rtx_mode_t with VOIDmode or BLKmode. Handling the CONST_WIDE_INT in dwarf2out.c the same as we did before (with CONST_DOUBLE) looks sensible as far of fixing a regression (I assume the diff to the dwarf results in essentially the same DWARF as what was present before wide-int). Richard. > Index: wide-int.h > =================================================================== > --- wide-int.h (revision 229720) > +++ wide-int.h (working copy) > @@ -156,6 +156,10 @@ > rtx r = ... > wide_int x = std::make_pair (r, mode); > > + If the mode is VOIDmode, then the precision of the wide_int will be > + the number of bits used to represent the rtl's value. This is used for > + untyped constants for dwarf debug information. > + > Similarly, a wide_int can only be constructed from a host value if > the target precision is given explicitly, such as in: > > The intent is to exactly match what gcc did before, to have wide-int handling match what const_int does, and what const_double did in the past, merely extended to support arbitrarily large values. I find, in this case, what was done in the past to be valid and desirable. I don’t find any reason to deviate from what was done before. > > The test case from the PR changes like this: > > --- t-bad.s 2015-11-03 16:19:13.786248224 -0500 > +++ t-fixed.s 2015-11-03 16:02:48.414290258 -0500 > @@ -23,7 +23,7 @@ test: > .Letext0: > .section .debug_info,"",@progbits > .Ldebug_info0: > - .long 0x63 # Length of Compilation Unit Info > + .long 0x74 # Length of Compilation Unit Info > .value 0x4 # DWARF version number > .long .Ldebug_abbrev0 # Offset Into Abbrev. Section > .byte 0x8 # Pointer Size (in bytes) > @@ -41,25 +41,28 @@ test: > .byte 0x1 # DW_AT_decl_file (t.c) > .byte 0x1 # DW_AT_decl_line > # DW_AT_prototyped > - .long 0x5a # DW_AT_type > + .long 0x6b # DW_AT_type > .quad .LFB0 # DW_AT_low_pc > .quad .LFE0-.LFB0 # DW_AT_high_pc > .uleb128 0x1 # DW_AT_frame_base > .byte 0x9c # DW_OP_call_frame_cfa > # DW_AT_GNU_all_call_sites > - .long 0x5a # DW_AT_sibling > + .long 0x6b # DW_AT_sibling > .uleb128 0x3 # (DIE (0x4e) DW_TAG_variable) > .long .LASF3 # DW_AT_name: "two127" > .byte 0x1 # DW_AT_decl_file (t.c) > .byte 0x3 # DW_AT_decl_line > - .long 0x61 # DW_AT_type > + .long 0x72 # DW_AT_type > + .byte 0x10 > + .quad 0 # DW_AT_const_value > + .quad 0x8000000000000000 # (null) > .byte 0 # end of children of DIE 0x2d > - .uleb128 0x4 # (DIE (0x5a) DW_TAG_base_type) > + .uleb128 0x4 # (DIE (0x6b) DW_TAG_base_type) > .byte 0x10 # DW_AT_byte_size > .byte 0x7 # DW_AT_encoding > .long .LASF4 # DW_AT_name: "__int128 unsigned" > - .uleb128 0x5 # (DIE (0x61) DW_TAG_const_type) > - .long 0x5a # DW_AT_type > + .uleb128 0x5 # (DIE (0x72) DW_TAG_const_type) > + .long 0x6b # DW_AT_type > .byte 0 # end of children of DIE 0xb > .section .debug_abbrev,"",@progbits > > I’m not a dwarf expert, but, I see the addition of two quads for a const value, and that value seems to be 1 << 127 to my untrained eye. If I had to theorizes, I’d say this is what the debug information was before. [ checking ] I checked gcc-4.8 on x86_64, and the debug information is now virtually the same: > > *** t-48.s 2015-11-03 16:24:23.286235021 -0500 > --- t-fixed.s 2015-11-03 16:02:48.414290258 -0500 > *************** test: > *** 28,35 **** > .long .Ldebug_abbrev0 # Offset Into Abbrev. Section > .byte 0x8 # Pointer Size (in bytes) > .uleb128 0x1 # (DIE (0xb) DW_TAG_compile_unit) > ! .long .LASF0 # DW_AT_producer: "GNU C 4.8.4 -mtune=generic -march=x86-64 -g -O -fstack-protector" > ! .byte 0x1 # DW_AT_language > .ascii "t.c\0" # DW_AT_name > .long .LASF1 # DW_AT_comp_dir: "/home/mrs/net/gcc-linuxO0/gcc" > .quad .Ltext0 # DW_AT_low_pc > --- 28,35 ---- > .long .Ldebug_abbrev0 # Offset Into Abbrev. Section > .byte 0x8 # Pointer Size (in bytes) > .uleb128 0x1 # (DIE (0xb) DW_TAG_compile_unit) > ! .long .LASF0 # DW_AT_producer: "GNU C11 6.0.0 20151103 (experimental) [trunk revision 229720] -O -g" > ! .byte 0xc # DW_AT_language > .ascii "t.c\0" # DW_AT_name > .long .LASF1 # DW_AT_comp_dir: "/home/mrs/net/gcc-linuxO0/gcc" > .quad .Ltext0 # DW_AT_low_pc > *************** test: > *** 55,61 **** > .long 0x72 # DW_AT_type > .byte 0x10 > .quad 0 # DW_AT_const_value > ! .quad 0x8000000000000000 > .byte 0 # end of children of DIE 0x2d > .uleb128 0x4 # (DIE (0x6b) DW_TAG_base_type) > .byte 0x10 # DW_AT_byte_size > --- 55,61 ---- > .long 0x72 # DW_AT_type > .byte 0x10 > .quad 0 # DW_AT_const_value > ! .quad 0x8000000000000000 # (null) > .byte 0 # end of children of DIE 0x2d > .uleb128 0x4 # (DIE (0x6b) DW_TAG_base_type) > .byte 0x10 # DW_AT_byte_size > *************** test: > *** 162,168 **** > .Ldebug_line0: > .section .debug_str,"MS",@progbits,1 > .LASF0: > ! .string "GNU C 4.8.4 -mtune=generic -march=x86-64 -g -O -fstack-protector" > .LASF1: > .string "/home/mrs/net/gcc-linuxO0/gcc" > .LASF3: > --- 162,168 ---- > .Ldebug_line0: > .section .debug_str,"MS",@progbits,1 > .LASF0: > ! .string "GNU C11 6.0.0 20151103 (experimental) [trunk revision 229720] -O -g" > .LASF1: > .string "/home/mrs/net/gcc-linuxO0/gcc" > .LASF3: > *************** test: > *** 171,175 **** > .string "test" > .LASF4: > .string "__int128 unsigned" > ! .ident "GCC: (Ubuntu 4.8.4-2ubuntu1~14.04) 4.8.4" > .section .note.GNU-stack,"",@progbits > --- 171,175 ---- > .string "test" > .LASF4: > .string "__int128 unsigned" > ! .ident "GCC: (GNU) 6.0.0 20151103 (experimental) [trunk revision 229720]" > .section .note.GNU-stack,"",@progbits > > The language seems weird, but, I bet it is inconsequential and an artifact of how I ran the test case. The # (null) is unfortunate, I don’t think this is better than what gcc-4.8 did, but, I don’t think that was caused by my patch. > > So, this would be my proposal to fix the issue. I'd invite anyone that thinks the dwarf information was wrong in gcc-4.8 or wrong post the patch, to let us know why. If there are reasons why my position is wrong, I’d like to hear about it, otherwise I’ll plan on checking this in with my wide-int hat on. Since this is a serious regression in debug quality, this has to go the the release branches that contain wide-int.
Index: rtl.h =================================================================== --- rtl.h (revision 229720) +++ rtl.h (working copy) @@ -2086,6 +2086,15 @@ inline unsigned int wi::int_traits <rtx_mode_t>::get_precision (const rtx_mode_t &x) { + /* VOIDmode CONST_WIDE_INT pairings are used for debug information + when the type of the underlying object has not been tracked and + is unimportant so we form a precision from the value of the + constant. This is sufficient for dwarf generation as dwarf can + track the type separately from the value. */ + if (x.second == VOIDmode + && CONST_WIDE_INT_P (x.first)) + return CONST_WIDE_INT_NUNITS (x.first) * HOST_BITS_PER_WIDE_INT; + return GET_MODE_PRECISION (x.second); } Index: wide-int.h =================================================================== --- wide-int.h (revision 229720) +++ wide-int.h (working copy) @@ -156,6 +156,10 @@ rtx r = ... wide_int x = std::make_pair (r, mode); + If the mode is VOIDmode, then the precision of the wide_int will be + the number of bits used to represent the rtl's value. This is used for + untyped constants for dwarf debug information. + Similarly, a wide_int can only be constructed from a host value if the target precision is given explicitly, such as in: The intent is to exactly match what gcc did before, to have wide-int handling match what const_int does, and what const_double did in the past, merely extended to support arbitrarily large values. I find, in this case, what was done in the past to be valid and desirable. I don’t find any reason to deviate from what was done before. The test case from the PR changes like this: --- t-bad.s 2015-11-03 16:19:13.786248224 -0500 +++ t-fixed.s 2015-11-03 16:02:48.414290258 -0500 @@ -23,7 +23,7 @@ test: .Letext0: .section .debug_info,"",@progbits .Ldebug_info0: - .long 0x63 # Length of Compilation Unit Info + .long 0x74 # Length of Compilation Unit Info .value 0x4 # DWARF version number .long .Ldebug_abbrev0 # Offset Into Abbrev. Section .byte 0x8 # Pointer Size (in bytes) @@ -41,25 +41,28 @@ test: .byte 0x1 # DW_AT_decl_file (t.c) .byte 0x1 # DW_AT_decl_line # DW_AT_prototyped - .long 0x5a # DW_AT_type + .long 0x6b # DW_AT_type .quad .LFB0 # DW_AT_low_pc .quad .LFE0-.LFB0 # DW_AT_high_pc .uleb128 0x1 # DW_AT_frame_base .byte 0x9c # DW_OP_call_frame_cfa # DW_AT_GNU_all_call_sites - .long 0x5a # DW_AT_sibling + .long 0x6b # DW_AT_sibling .uleb128 0x3 # (DIE (0x4e) DW_TAG_variable) .long .LASF3 # DW_AT_name: "two127" .byte 0x1 # DW_AT_decl_file (t.c) .byte 0x3 # DW_AT_decl_line - .long 0x61 # DW_AT_type + .long 0x72 # DW_AT_type + .byte 0x10 + .quad 0 # DW_AT_const_value + .quad 0x8000000000000000 # (null) .byte 0 # end of children of DIE 0x2d - .uleb128 0x4 # (DIE (0x5a) DW_TAG_base_type) + .uleb128 0x4 # (DIE (0x6b) DW_TAG_base_type) .byte 0x10 # DW_AT_byte_size .byte 0x7 # DW_AT_encoding .long .LASF4 # DW_AT_name: "__int128 unsigned" - .uleb128 0x5 # (DIE (0x61) DW_TAG_const_type) - .long 0x5a # DW_AT_type + .uleb128 0x5 # (DIE (0x72) DW_TAG_const_type) + .long 0x6b # DW_AT_type .byte 0 # end of children of DIE 0xb .section .debug_abbrev,"",@progbits