Re: bitpos expansion patches summary

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

On Sun, 2 Sep 2012 20:15:15 +0200, Jan wrote:
> I was thinking about some updating of file:lineno records in
> '.locdiff.report' file to their new location and then diffing the
> old+reviewed '.locdiff.report' against new file.  But I have not
> tried it yet in practice.

This will be heuristic to some extent, which is why I avoided it. 

> FIXED(Expand len):
> (mips-tdep.c:4701):	VARINIT(len):	(ULONGEST to
> int)	[(arg_type)->length]
>  - You have fixed line 4646 but I do not see line 4701 to be fixed.

Probably a case of different line numbers due to using a different
updated version? I was rebasing every now and then, which gave me
different line numbers for some warnings. I see this on line 4695 now.

> FIXED(expand offset): (mips-tdep.c:5040):	CMP:
> (ULONGEST to int)	[offset + xfer > (type)->length]
>  - I do not see a need for this change, it is all limited by:
>    TYPE_LENGTH (type) <= 2 * MIPS64_REGSIZE

Right, reverted.

> SAFE(stack_dest should
> be CORE_ADDR, but safe for now): (mt-tdep.c:786):
> VARINIT(stack_dest):	(CORE_ADDR to LONGEST)	[sp]
>  - There should be checked overflow of sp - length in general in these
>    *_push_dummy_call functions but passing >2GB structs by value on
> stack is probably not worth fixing.
>    But I do not see why you did not choose CORE_ADDR for stack_dest

Because it would change the sign, which is something we agreed we don't
want to do for this patch.

> UNSAFE_ALLOCA: (mt-tdep.c:852):
> FUNC(C_alloca):	(LONGEST to size_t)	[typelen + slacklen]
>  - Just drop alloca, two write_memory calls are enough.

This has already been replaced with xmalloc/xfree in cvs. But yes, two
write_memory calls should have been enough.

> UNSAFE_ALLOCA: (mt-tdep.c:853):	FUNC(memcpy):	(LONGEST
> to size_t)	[typelen]
>  - Just drop alloca, two write_memory calls are enough.

Likewise.

> FIXED(Expand startrest):
> (opencl-lang.c:253):	VARINIT(startrest):	(LONGEST to
> int)	[offset % elsize]
>  - 'elsize' line is longer than 80 columns.  One should check the
> whole patch. (In these cases a new helper variable makes it easier to
> conform to the GNU indentation style.)

OK, fixed.

> SAFE: (opencl-lang.c:428):	FUNC(memcpy):	(ULONGEST to
> size_t)	[(elm_type)->length]
>  - I miss here FIX of: opencl-lang.c:450
>    int src_len; LONGEST lowb, highb; src_len = highb - lowb + 1;
>    It will affect also at least create_value().

This is called only for a vector, so highb - lowb should be bound by
vector size. You made a point later about gcc possibly generating
incorrect code and hence giving us a large value. Do you want me to
expand this to cater for such a situation?

> SAFE: (opencl-lang.c:681):
> FUNC(memset):	(ULONGEST to size_t)	[(eltype1)->length]
>  - I miss here opencl-lang.c:895 where variable "i" should have been
> expanded. int i; LONGEST lowb1, highb1; for (i = 0; i < highb1 -
> lowb1 + 1; i++) 

Again, this should be limited by vector size.

> SAFE:
> (ppc-sysv-tdep.c:162):	FUNC(align_up):	(LONGEST to
> int)	[len]
>  - I would say at ppc-sysv-tdep.c:124 'len' it should be int->ssize_t
> instead of int->LONGEST as there is already value_contents making the
> checks somehow easier. 

OK, fixed.

> SAFE(Vectors may not be larger than int):
> (ppc-sysv-tdep.c:447):	CMP:	(LONGEST to int)	[i
> < len / 16]
>  - This is dependency on external producer (gcc).
>    GDB should verify it, such as ENSURE_SIZET or a proper handling.
>    External producer could incorrectly mark DW_AT_GNU_vector some
> large type and GDB would process it incorrectly.

OK, fixed.

> REVERTED(vectors < 16 bytes):
> (ppc-sysv-tdep.c:852):	VARINIT(regnum):	(LONGEST to
> int)	[tdep->ppc_fp0_regnum + 1 + i]
>  - it still could REVERT ppc-sysv-tdep.c:844 and ppc-sysv-tdep.c:848.

OK.

> SAFE(int
> sufficient for OpenCL vectors): (ppc-sysv-tdep.c:897):
> VARINIT(n_regs):	(ULONGEST to int)	[(type)->length / 16]
>  - While true again it is dependency on external producer (gcc).

OK.

> SAFE(OpenCL Vector):
> (ppc-sysv-tdep.c:1448):	CMP:	(ULONGEST to int)
> [i < (type)->length / 16]
>  - While true again it is dependency on external producer (gcc).

OK.

> FIXED(Expand byte):
> (ppc-sysv-tdep.c:1561):	CMP:	(ULONGEST to int)
> [byte < (type)->length]
>  - OK although ssize_t would be enough here.

OK.

> FIXED(Expand len): (ppc-sysv-tdep.c:1567):
> VARINIT(len):	(ULONGEST to int)	[(type)->length - byte]
>  - OK although ssize_t would be enough here.

OK.

> SAFE: (ppc-sysv-tdep.c:1597):	FUNC(write_memory):
> (LONGEST to ssize_t)	[len]
>  - OK although ssize_t would be enough here.

OK.

> > [(valtype)->length] SAFE: (printcmd.c:2269):
> > VARINIT(param_len):	(ULONGEST to UINT)
> > [(param_type)->length]
>  - While it is technically right I would expand it here, at the point
> where it is computed it is still not clear the type is
> TYPE_CODE_DECFLOAT.

I am actually more inclined to eliminate the single-use variable and use
TYPE_LENGTH directly. What do you think?

> SAFE: (p-valprint.c:212):
> FUNC(xmalloc):	(LONGEST to size_t)	[length_size]
>  - Missing ENSURED_SIZET.  Normally it cannot happen but broken
> compiler can produce arbitrarily long length-of-string field which
> this way could be incorrectly processed by GDB without an error.
>  - extract_unsigned_integer may also need similar handling as size_t
> may be > int. 

OK, will do.

> SAFE: (p-valprint.c:323):	ASSIGN:	(ULONGEST to
> UINT)	[ len = extract_unsigned_integer(valaddr +
> embedded_offset + length_pos, length_size, byte_order)]
>  - Again, like above.

OK.

> [(type)->length] FIXED(Expand n): (s390-tdep.c:2516):
> VARINIT(length):	(ULONGEST to UINT)	[(type)->length]
>  - I do not see 'n' anywhere, you have modified

Looks like I missed this due to some misplaced comment. length needs to
be expanded instead here. Fixed.

> FIXED(Expand len, sh_justify_value_in_reg arg):
> (sh-tdep.c:1097):	ASSIGN:	(ULONGEST to int)
> [ len = (type)->length]
>  - Excessive ensure_type_fits_sizet in your patch at sh-tdep.c:1129
>    because there is sh_justify_value_in_reg above it.

I think I have removed this in a later version of the patch.

> [(type)->length == 2 * reg_size] FIXED(Expand len,
> sh_justify_value_in_reg arg): (sh-tdep.c:1234):	ASSIGN:
> (ULONGEST to int)	[ len = (type)->length]
>  - Excessive ensure_type_fits_sizet in your patch at sh-tdep.c:1258
>    because there is sh_justify_value_in_reg above it.

Likewise.

> - len : 0] FIXED(Expand len): (spu-tdep.c:1296):
> VARINIT(len):	(ULONGEST to int)	[(type)->length]
>  - This is not needed, it has to fit in inferior registers.

spu_push_dummy_call also calls spu_value_to_regcache with a type
without checking the size of the type.

> FIXED(Likewise): (spu-tdep.c:1321):	VARINIT(len):
> (ULONGEST to int)	[(type)->length]
>  - Likewise.

OK, fixed.

> FIXED(Expand highest_offset, start,
> print_frame_nameless_args arg 2): (stack.c:548):
> ASSIGN:	(LONGEST to LONG)	[ highest_offset =
> current_offset]
>  - current_offset/highest_offset could be split out, it is a
> different bug. But fine with merged it here.

I don't know what you mean by splitting it out - could you please
elaborate?

> FIXED(Expand len):
> (tic6x-tdep.c:974):	VARINIT(len):	(ULONGEST to
> int)	[(arg_type)->length]
>  - references_offset is incorrectly not expanded.

OK.

> FIXED(Expand len, i):
> (tilegx-tdep.c:221):	VARINIT(len):	(ULONGEST to
> int)	[(type)->length]
>  - not needed, it is used only if !tilegx_use_struct_convention

OK.

> FIXED(Expand len, i):
> (tilegx-tdep.c:246):	VARINIT(len):	(ULONGEST to
> int)	[(type)->length]
>  - not needed, it is used only if !tilegx_use_struct_convention

OK.

> FIXED(Expand typelen): (tilegx-tdep.c:308):	ASSIGN:
> (ULONGEST to int)	[ typelen =
> (value_enclosing_type(args[i]))->length]
>  - For example stacklen and alignlen are incorrectly not expanded.

OK.

> UNSAFE_ALLOCA: (tilegx-tdep.c:347):	ASSIGN:	(ULONGEST
> to int)	[ typelen = (value_enclosing_type(args[j]))->length]
>  - Please fix as an unrelated patch; it should be also ENSURED

Already done.

> FIXED: (tracepoint.c:1485):	FUNC(add_memrange):
> (ULONGEST to ULONG)	[len]
>  - Expansion of 'addr' is correct but unrelated.

OK. I'll keep it as is since it is trivial anyway.

> FIXED(Expand len): (v850-tdep.c:851):	ASSIGN:	(ULONGEST
> to int)	[ len = (value_type(*args))->length]
>  - Not needed to expand v850-tdep.c:893 due
> to !v850_use_struct_convention by its caller.
>  - Not needed to expand v850-tdep.c:920 due
> to !v850_use_struct_convention by its caller. 

OK.

> SAFE:
> (valarith.c:757):	ASSIGN:	(ULONGEST to int)
> [ inval1len = (type1)->length]
>  - TYPE_CODE_STRING can be also >2GB, it is in fact a sort of array.
>    For example Fortran uses these.
>    Also it needs alloca expansion.

OK will do.

> SAFE: (valarith.c:758):	ASSIGN:	(ULONGEST to
> int)	[ inval2len = (type2)->length]
>  - Likewise.

OK.

> FIXED(Expanded put_frame_register_bytes arg 4):
> (valops.c:1373):	FUNC(put_frame_register_bytes):
> (ULONGEST to int)	[(type)->length]
>  - I do not see any expansion and I do not see why.
>    lval_register values can never be too large.

OK, I missed it and I was right ;)

> FIXED(Expand value_cstring arg
> 2, highbound to LONGEST): (valops.c:1849):
> VARINIT(highbound):	(ULONGEST to int)	[len /
> (char_type)->length]
>  - value_cstring's parameter 'int->LONGEST len' should have been only
> ssize_t. It cannot be larger than 'char *ptr' memory.

OK.

> size_t)	[len] FIXED: (valops.c:1872):
> VARINIT(highbound):	(ULONGEST to int)	[len /
> (char_type)->length]
>  - Likewise for the 'len' parameter.

OK.

> SAFE: (valops.c:1891):
> FUNC(memcpy):	(ULONGEST to size_t)	[(type)->length]
>  - While TYPE_CODE_BITSTRING is not used in practice I do not see why
> it should not be expanded(=fixed).

I don't get this. I think I have the wrong line numbers and hence not
able to see what you're seeing.


Regards,
Siddhesh