On Tue, Nov 3, 2015 at 10:58 PM, Mike Stump <mikest...@comcast.net> wrote:
> On Nov 3, 2015, at 12:46 AM, Richard Sandiford <richard.sandif...@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.
