Undefined behavior or gcc is doing additional good job?

Undefined behavior or gcc is doing additional good job?

[Bin.Cheng]

Hi, For below simple example:
#include <stdint.h>

extern uint32_t __bss_start[];
extern uint32_t __data_start[];

void Reset_Handler(void)
{
 /* Clear .bss section (initialize with zeros) */
 for (uint32_t* bss_ptr = __bss_start; bss_ptr != __data_start; ++bss_ptr) {
  *bss_ptr = 0;
 }
}

One snapshot of our branch generates:
Reset_Handler:
    @ args = 0, pretend = 0, frame = 0
    @ frame_needed = 0, uses_anonymous_args = 0
    @ link register save eliminated.
    ldr    r2, .L6
    ldr    r1, .L6+4
    subs    r1, r1, r2
    bic    r1, r1, #3
    movs    r3, #0
.L2:
    cmp    r3, r1
    beq    .L5
    movs    r0, #0
    str    r0, [r2, r3]
    adds    r3, r3, #4
    b    .L2
.L5:
    bx    lr
.L7:
    .align    2
.L6:
    .word    __bss_start
    .word    __data_start
    .size    Reset_Handler, .-Reset_Handler

I know the IVOPT chooses wrong candidate here, but what I am not sure about is:
0) the original code is not safe. It could result in infinite loop if
there is any alignment issue of __bss_ptr and __data_start.
1) GCC explicitly clears the two lower bits of (__bss_ptr -
__data_start). This makes the loop safe (from infinite loop).

My question is, is it intended for GCC to do such transformation?

Thanks,
bin
-- 
Best Regards.

Re: Undefined behavior or gcc is doing additional good job?

[Jakub Jelinek]

On Fri, Jan 03, 2014 at 04:12:19PM +0800, Bin.Cheng wrote:
> Hi, For below simple example:
> #include <stdint.h>
> 
> extern uint32_t __bss_start[];
> extern uint32_t __data_start[];
> 
> void Reset_Handler(void)
> {
>  /* Clear .bss section (initialize with zeros) */
>  for (uint32_t* bss_ptr = __bss_start; bss_ptr != __data_start; ++bss_ptr) {
>   *bss_ptr = 0;
>  }
> }

I believe this is undefined behavior, so GCC can assume
bss_ptr != __data_start is true always.  You need something like
memset (__bss_start, 0, (uintptr_t) __data_start - (uintptr_t) __bss_start);
(note the cases to non-pointers), then it is just implementation defined
behavior.  Or do
uint32_t data_ptr;
asm ("" : "g" (data_ptr) : "0" (__data_start));
for (uint32_t* bss_ptr = __bss_start; bss_ptr != data_ptr; ++bss_ptr) {
  *bss_ptr = 0;
}
and thus hide from the compiler the fact that __data_start is in a different
object.

	Jakub

Re: Undefined behavior or gcc is doing additional good job?

[Bin.Cheng]

On Fri, Jan 3, 2014 at 4:24 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Jan 03, 2014 at 04:12:19PM +0800, Bin.Cheng wrote:
>> Hi, For below simple example:
>> #include <stdint.h>
>>
>> extern uint32_t __bss_start[];
>> extern uint32_t __data_start[];
>>
>> void Reset_Handler(void)
>> {
>>  /* Clear .bss section (initialize with zeros) */
>>  for (uint32_t* bss_ptr = __bss_start; bss_ptr != __data_start; ++bss_ptr) {
>>   *bss_ptr = 0;
>>  }
>> }
>
> I believe this is undefined behavior, so GCC can assume
> bss_ptr != __data_start is true always.  You need something like
Sorry for posting the premature question.  Since both __bss_start and
__data_start are declared as array, it seems there is no undefined
behavior, the check is between two pointers with same type actually,
right?  So the question remains, why GCC would clear the two lower
bits of " __data_start - __bss_start" then?  Am I some stupid mistake?

Thanks,
bin

> memset (__bss_start, 0, (uintptr_t) __data_start - (uintptr_t) __bss_start);
> (note the cases to non-pointers), then it is just implementation defined
> behavior.  Or do
> uint32_t data_ptr;
> asm ("" : "g" (data_ptr) : "0" (__data_start));
> for (uint32_t* bss_ptr = __bss_start; bss_ptr != data_ptr; ++bss_ptr) {
>   *bss_ptr = 0;
> }
> and thus hide from the compiler the fact that __data_start is in a different
> object.
>
>         Jakub



-- 
Best Regards.

Re: Undefined behavior or gcc is doing additional good job?

[Jakub Jelinek]

On Fri, Jan 03, 2014 at 04:44:48PM +0800, Bin.Cheng wrote:
> >> extern uint32_t __bss_start[];
> >> extern uint32_t __data_start[];
> >>
> >> void Reset_Handler(void)
> >> {
> >>  /* Clear .bss section (initialize with zeros) */
> >>  for (uint32_t* bss_ptr = __bss_start; bss_ptr != __data_start; ++bss_ptr) {
> >>   *bss_ptr = 0;
> >>  }
> >> }
> >
> > I believe this is undefined behavior, so GCC can assume
> > bss_ptr != __data_start is true always.  You need something like
> Sorry for posting the premature question.  Since both __bss_start and
> __data_start are declared as array, it seems there is no undefined
> behavior, the check is between two pointers with same type actually,

I think this has been discussed in some PR, unfortunately I can't find it.
If it was < or <=, then it would be obvious undefined behavior, those
comparisons can't be performed between different objects, the above is
questionable, because you still assume that you get through pointer
arithmetics from one object to another one, without dereference pointer
arithmetics can be at one past last entry in the array, but whether that is
equal to the object object is still quite problematic.

> right?  So the question remains, why GCC would clear the two lower
> bits of " __data_start - __bss_start" then?  Am I some stupid mistake?

That said, if either of __bss_start of __data_start aren't 32-bit aligned,
then it is a clear undefined behavior, the masking of low 2 bits (doesn't
happen on x86_64) comes from IVopts computing the end as
((__data_start - __bss_start) + 1) * 4 and the __data_start - __bss_start
is exact division by 4, apparently we don't fold that back to just
(char *) __data_start - (char *) __bss_start + 4.

	Jakub

Re: Undefined behavior or gcc is doing additional good job?

[Joseph S. Myers]

On Fri, 3 Jan 2014, Jakub Jelinek wrote:

> I think this has been discussed in some PR, unfortunately I can't find it.

Bug 57725?

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Undefined behavior or gcc is doing additional good job?

[Bin.Cheng]

On Fri, Jan 3, 2014 at 5:17 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Jan 03, 2014 at 04:44:48PM +0800, Bin.Cheng wrote:
>> >> extern uint32_t __bss_start[];
>> >> extern uint32_t __data_start[];
>> >>
>> >> void Reset_Handler(void)
>> >> {
>> >>  /* Clear .bss section (initialize with zeros) */
>> >>  for (uint32_t* bss_ptr = __bss_start; bss_ptr != __data_start; ++bss_ptr) {
>> >>   *bss_ptr = 0;
>> >>  }
>> >> }
>> >
>> > I believe this is undefined behavior, so GCC can assume
>> > bss_ptr != __data_start is true always.  You need something like
>> Sorry for posting the premature question.  Since both __bss_start and
>> __data_start are declared as array, it seems there is no undefined
>> behavior, the check is between two pointers with same type actually,
>
> I think this has been discussed in some PR, unfortunately I can't find it.
> If it was < or <=, then it would be obvious undefined behavior, those
> comparisons can't be performed between different objects, the above is
> questionable, because you still assume that you get through pointer
> arithmetics from one object to another one, without dereference pointer
> arithmetics can be at one past last entry in the array, but whether that is
> equal to the object object is still quite problematic.
Sorry for late replying.
It seems equality operators allow two pointers to compare equal if one
pointer is a pointer to one past the end of one array object and the
other is a pointer to the start of a different array object that
happens to immediately follow the first array object in the address
space.  Then the "!=" condition can't be always true in the example.

>
>> right?  So the question remains, why GCC would clear the two lower
>> bits of " __data_start - __bss_start" then?  Am I some stupid mistake?
>
> That said, if either of __bss_start of __data_start aren't 32-bit aligned,
> then it is a clear undefined behavior, the masking of low 2 bits (doesn't
> happen on x86_64) comes from IVopts computing the end as
> ((__data_start - __bss_start) + 1) * 4 and the __data_start - __bss_start
> is exact division by 4, apparently we don't fold that back to just
> (char *) __data_start - (char *) __bss_start + 4.
Em, YES, it comes from ivopt rewriting, but, if it's not undefined
behavior, won't it be annoying (or simply wrong) for compiler to do
something not written by the code?

Thanks,
bin


-- 
Best Regards.

Re: Undefined behavior or gcc is doing additional good job?

[Jakub Jelinek]

On Tue, Jan 07, 2014 at 04:01:23PM +0800, Bin.Cheng wrote:
> Em, YES, it comes from ivopt rewriting, but, if it's not undefined
> behavior, won't it be annoying (or simply wrong) for compiler to do
> something not written by the code?

If __bss_start of __data_start aren't 32-bit aligned, then it is undefined
behavior, so it is not wrong.

	Jakub

Re: Undefined behavior or gcc is doing additional good job?

[Bin.Cheng]

On Tue, Jan 7, 2014 at 4:10 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Jan 07, 2014 at 04:01:23PM +0800, Bin.Cheng wrote:
>> Em, YES, it comes from ivopt rewriting, but, if it's not undefined
>> behavior, won't it be annoying (or simply wrong) for compiler to do
>> something not written by the code?
>
> If __bss_start of __data_start aren't 32-bit aligned, then it is undefined
> behavior, so it is not wrong.
I see.  Thanks for elaborating.

Thanks,
bin

-- 
Best Regards.