Re: bitpos expansion patches summary

Siddhesh Poyarekar
2012-09-07T10:52:57Z

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