On 05/07/16 01:33, David Wohlferd wrote:
>
>>> 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.
>
> Seems fair. You are the one putting the time in to change it.
>
> But do you have actual code that is affected by this? You can't really
> be planning to wait until v7 is released to have your projects work
> correctly?
>
No, I can wait.
This is a long term investment in overall software reliability.
>> 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.
>
> I'm not sure I follow. Do you fear that gcc could mistakenly move the
> asm into a nearby loop during optimization (resulting in who-knows-what
> results)? Or is there some way that any basic asm in a loop could have
> some kind of a problem?
>
It is a risk, that who-knows-what will happen, unexpectedly.
Bernd.
>> 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.
>
> According to the docs, that is only supported for global registers. The
> docs for local register variables explicitly say that it can't be used
> as input/outputs for basic asm.
>
>> 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.
>
> I think we should try to avoid changing the semantics in v7 for memory
> and then changing them again in v8 for registers.
>
> IOW, if I see some basic asm in a project (or on stack overflow/blog as
> a code fragment), should I assume it was intended for v6 semantics? v7?
> v8? People often copy this stuff without understanding what it does.
> The more often the semantics change, the harder it is to use correctly
> and maintain.
>
>>> 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.
>
> After your patch has been checked in, I'll re-submit this.
>
>>> 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."
>
> Is it worth describing the fact that the semantics have changed here?
> Something like "Before v7, gcc assumed no changes were made to memory."
> I guess users can "figure it out" by reading the v6 docs and comparing
> it to v7. But if the semantic change has introduced a problem that
> someone is trying to debug, this could help them track it down.
>
> Also, I'm kind of hoping that v7 is the 'future release.' If we are
> going to change the clobbers, I'd hope that we'd do it all at one time,
> rather than spreading it out across several releases.
>
> If no one is prepared to step up and implement (or at least defend) the
> idea of clobbering registers, I'd like to see the "This assumption may
> change in a future release" part removed. Since (theoretically)
> anything can change at any time, the only reason this text made sense
> was because a change was imminent. If that's no longer true, it's time
> for it to go.
>
> dw
