On 05/06/16 08:35, David Wohlferd wrote:
> On 5/5/2016 10:29 AM, Bernd Edlinger wrote:
>> Hi!
>>
>> this patch is inspired by recent discussion about basic asm:
>>
>> Currently a basic asm is an instruction scheduling barrier,
>> but not a memory barrier, and most surprising, basic asm
>> does _not_ implicitly clobber CC on targets where
>> extended asm always implicitly clobbers CC, even if
>> nothing is in the clobber section.
>>
>> This patch makes basic asm implicitly clobber CC on certain
>> targets, and makes the basic asm implicitly clobber memory,
>> but no general registers, which is what could be expected.
>>
>> This is however only done for basic asm with non-empty
>> assembler string, which is in sync with David's proposed
>> basic asm warnings patch.
>>
>> Due to the change in the tree representation, where
>> ASM_INPUT can now be the first element of a
>> PARALLEL block with the implicit clobber elements,
>> there are some changes necessary.
>>
>> Most of the changes in the middle end, were necessary
>> because extract_asm_operands can not be used to find out
>> if a PARALLEL block is an asm statement, but in most cases
>> asm_noperands can be used instead.
>>
>> There are also changes necessary in two targets: pa, and ia64.
>> I have successfully built cross-compilers for these targets.
>>
>> Boot-strapped and reg-tested on x86_64-pc-linux-gnu
>> OK for trunk?
>
> A few questions:
>
> 1) I'm not clear precisely what problem this patch fixes. It's true
> that some people have incorrectly assumed that basic asm clobbers memory
> and this change would fix their code. But some people also incorrectly
> assume it clobbers registers. I assume that's why Jeff Law proposed
> making basic asm "an opaque blob that read/write/clobber any register or
> memory location." Do we have enough problem reports from users to know
> which is the real solution here?
>
Whenever I do something for gcc I do it actually for myself, in my own
best interest. And this is no exception.
The way I see it, is this: in simple cases a basic asm behaves as if
it would clobber memory, because of the way Jeff implemented the
asm handling in sched-deps.c some 20 years ago.
Look for ASM_INPUT where we have this comment:
"Traditional and volatile asm instructions must be considered to use
and clobber all hard registers, all pseudo-registers and all of
memory."
The assumption that it is OK to clobber memory in a basic asm will only
break if the asm statement is inlined in a loop, and that may happen
unexpectedly, when gcc rolls out new optimizations.
That's why I consider this to be security relevant.
But OTOH you see immediately that all general registers are in use
by gcc, unless you declare a variable like
register int eax __asm__("rax");
then it is perfectly OK to use rax in a basic asm of course.
And if we want to have implicitly clobbered registers, like the
diab compiler handles the basic asm, then this patch will
make it possible to add a target hook that clobbers additional
registers for basic asm.
> 2) The -Wbasic-asm warning patch wasn't approved for v6. If we are
> going to change this behavior now, is it time?
>
Yes. We have stage1 for gcc-7 development, I can't think of a better
time for it.
I would even say, the -Wbasic-asm warning patch makes more sense now,
because we could warn, that the basich asm clobbers memory, which it
did not previously.
> 3) I assume there are good reasons why extended asm can't be used at top
> level. Will adding these clobbers cause those problems in basic asm too?
>
No, these don't come along here, and nothing should change for them.
> 4) There are more basic asm docs that need to change: "It also does not
> know about side effects of the assembler code, such as modifications to
> memory or registers. Unlike some compilers, GCC assumes that no changes
> to either memory or registers occur. This assumption may change in a
> future release."
>
Yes, I should change that sentence too.
Maybe this way:
"Unlike some compilers, GCC assumes that no changes to registers
occur. This assumption may change in a future release."
Thanks
Bernd.
> dw
