This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Avoid negative bitpos in expand_expr_real_1 (PR 86984)


On 08/20/18 12:41, Richard Biener wrote:
> On Sun, 19 Aug 2018, Bernd Edlinger wrote:
> 
>> Hi!
>>
>>
>> This fixes a wrong code issue in expand_expr_real_1 which happens because
>> a negative bitpos is actually able to reach extract_bit_field which
>> does all computations with poly_uint64, thus the offset 0x1ffffffffffffff0.
>>
>> To avoid that I propose to use Jakub's r204444 patch from the expand_assignment
>> also in the expand_expr_real_1.
>>
>> This is a rather unlikely thing to happen, as there are lots of checks that are
>> of course all target dependent between the get_inner_reference and the
>> actual extract_bit_field call, and all other code paths may or may not have a problem
>> with negative bit offsets.  Most don't have a problem, but the bitpos needs to be
>> folded into offset before it is used, therefore it is necessary to handle the negative
>> bitpos very far away from the extract_bit_field call.  Doing that later is IMHO not
>> possible.
>>
>> The fix in CONSTANT_ADDRESS_P is actually unrelated, and I only spotted it because
>> this macro is used in alpha_legitimize_address which is of course what one looks
>> at first if something like that happens.
>>
>> I think even with this bogus offset it should not have caused a linker error, so there
>> is probably a second problem in the *movdi code pattern of the alpha.md, because it
>> should be split into instructions that don't cause a link error.
>>
>> Once the target is fixed to split the impossible assembler instruction, the test case
>> will probably no longer be able to detect the pattern in the assembly.
>>
>> Therefore the test case looks both at the assembler output and the expand rtl dump
>> to spot the bogus offset.  I only check the first 12 digits of the bogus constant,
>> because it is actually dependent on the target configuration:
>>
>> I built first a cross-compiler without binutils, and it did used a slightly different
>> offset of 0x2000000000000000, (configured with: --target=alpha-linux-gnu --enable-languages=c
>> --disable-libgcc --disable-libssp --disable-libquadmath --disable-libgomp --disable-libatomic)
>> when the binutils are installed at where --prefix points, the offset is 0x1ffffffffffffff0.
>>
>> Regarding the alpha target, I could not do more than build a cross compiler and run
>> make check-gcc-c RUNTESTFLAGS="alpha.exp=pr86984.c".
>>
>>
>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>> Is it OK for trunk?
> 
> Hmm, I don't remember why we are inconsistent in get_inner_reference
> with respect to negative bitpos.  I think we should be consistent
> here and may not be only by accident?  That is, does
> 
> diff --git a/gcc/expr.c b/gcc/expr.c
> index c071be67783..9dc78587136 100644
> --- a/gcc/expr.c
> +++ b/gcc/expr.c
> @@ -7272,7 +7272,7 @@ get_inner_reference (tree exp, poly_int64_pod
> *pbitsize,
>                                        TYPE_PRECISION (sizetype));
>         tem <<= LOG2_BITS_PER_UNIT;
>         tem += bit_offset;
> -      if (tem.to_shwi (pbitpos))
> +      if (tem.to_shwi (pbitpos) && !maybe_lt (*pbitpos, 0))
>          *poffset = offset = NULL_TREE;
>       }
>   
> fix the issue?
> 

Yes, at first sight, however, I was involved at PR 58970,
see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58970

and I proposed a similar patch, which was objected by Jakub:

see comment #25 of PR 58970:
"Jakub Jelinek 2013-11-05 19:41:12 UTC

(In reply to Bernd Edlinger from comment #24)
> Created attachment 31169 [details] https://gcc.gnu.org/bugzilla/attachment.cgi?id=31169
> Another (better) attempt at fixing this ICE.
> 
> This avoids any negative bitpos from get_inner_reference.
> These negative bitpos values are just _very_ dangerous all the
> way down to expmed.c

I disagree that it is better, you are forgetting get_inner_reference has other > 20 callers outside of expansion and there is no reason why negative bitpos would be a problem in those cases."

So that is what Jakub said at that time, and with the
suggested change in get_inner_reference,
this part of the r204444 change would be effectively become superfluous:

@@ -4721,6 +4721,15 @@ expand_assignment (tree to, tree from, bool nontem
        tem = get_inner_reference (to, &bitsize, &bitpos, &offset, &mode1,
                                  &unsignedp, &volatilep, true);
  
+      /* Make sure bitpos is not negative, it can wreak havoc later.  */
+      if (bitpos < 0)
+       {
+         gcc_assert (offset == NULL_TREE);
+         offset = size_int (bitpos >> (BITS_PER_UNIT == 8
+                                       ? 3 : exact_log2 (BITS_PER_UNIT)));
+         bitpos &= BITS_PER_UNIT - 1;
+       }
+
        if (TREE_CODE (to) == COMPONENT_REF
           && DECL_BIT_FIELD_TYPE (TREE_OPERAND (to, 1)))
         get_bit_range (&bitregion_start, &bitregion_end, to, &bitpos, &offset);

and should be reverted.  I did not really like it then, but I'd respect Jakub's advice.


Bernd.



> Richard.
> 

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]