Re: [PATCH] Make basic asm implicitly clobber memory
On 05/23/16 23:46, David Wohlferd wrote:
> On 5/23/2016 12:46 AM, Richard Biener wrote:
> > On Sun, 22 May 2016, Andrew Haley wrote:
> >> On 05/20/2016 07:50 AM, David Wohlferd wrote:
> >>> I realize deprecation/removal is drastic. Especially since basic
> >>> asm (mostly) works as is. But fixing memory clobbers while leaving
> >>> the rest broken feels like half a solution, meaning that some day
> >>> we're going to have to fiddle with this again.
> >>
> >> Yes, we will undoubtedly have to fiddle with basic asm again. We
> >> should plan for deprecation.
> >
> > I think adding memory clobbers is worth having. I also think that
> > deprecating basic asms would be a good thing, so can we please
> > add a new warning for that? "warning: basic asms are deprecated"
>
> I've still got the -Wbasic-asm patch where I proposed this for v6. I can
> dust it off again and re-submit it. A couple questions first:
>
> 1) In this patch the warning was disabled by default. But it sounds
> like you want it enabled by default? Easy to change, I'm just
> confirming your intent.
>
For practical reasons I would suggest to enable a warning like that,
only with -Wall otherwise you would have to decorate lots of test cases
with dg-warning statements (and it is rather difficult to do that for
all affected targets).
> 2) Is 'deprecated' handled differently than other types of warnings?
> There is a -Wno-deprecated, but it seems to have a very specific meaning
> that does not apply here.
>
> 3) The warning text in the old patch was "asm statement in function does
> not use extended syntax". The intent was:
>
> a) Don't make it sound like basic asm is completely gone, since it can
> still be used at top level.
> b) Don't make it sound like all inline asm is gone, since extended asm
> can still be used in functions.
> c) Convey all that in as few words as possible.
>
The warning could also mention the changed behavior regarding the memory
clobbers, and recommend using extended asm syntax for that reason.
That was at least my initial thought.
> Now that we want to add the word 'deprecated,' perhaps one of these:
>
> - Basic asm in functions is deprecated in favor of extended syntax
> - asm in functions without extended syntax is deprecated
> - Deprecated: basic asm in function
> - Deprecated: asm in function without extended syntax
>
> I like the last one (people may not know what 'basic' means in this
> context), but any of these would work for me. Preferences?
>
> In order to avoid conflicts, I'll wait for Bernd to commit his patch first.
>
Maybe we should not deprecate every use case, asm("") was fine, for
certain reasons.
Furthermore I think the ia64 port could still theoretically use
traditional asm to specify the stop bits (see config/ia64/ia64.c,
rtx_needs_barrier).
BTW: My patch still waits to be reviewed in detail by one of the global
reviewers, before I can apply it.
Meanwhile I added this to doc/extend.texi, in response to David's
comments:
--- gcc/doc/extend.texi (revision 231412)
+++ gcc/doc/extend.texi (working copy)
@@ -7508,7 +7508,7 @@
inside them. GCC has no visibility of symbols in the @code{asm} and may
discard them as unreferenced. 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
+some compilers, GCC assumes that no changes to general purpose registers
occur. This assumption may change in a future release.
To avoid complications from future changes to the semantics and the
Which is just the fact. Obviously the doc will need further
polishing though, I'd like to leave that to David.
Thanks
Bernd.
Re: [PATCH] Make basic asm implicitly clobber memory
On 5/23/2016 12:46 AM, Richard Biener wrote: > On Sun, 22 May 2016, Andrew Haley wrote: >> On 05/20/2016 07:50 AM, David Wohlferd wrote: >>> I realize deprecation/removal is drastic. Especially since basic >>> asm (mostly) works as is. But fixing memory clobbers while leaving >>> the rest broken feels like half a solution, meaning that some day >>> we're going to have to fiddle with this again. >> >> Yes, we will undoubtedly have to fiddle with basic asm again. We >> should plan for deprecation. > > I think adding memory clobbers is worth having. I also think that > deprecating basic asms would be a good thing, so can we please > add a new warning for that? "warning: basic asms are deprecated" I've still got the -Wbasic-asm patch where I proposed this for v6. I can dust it off again and re-submit it. A couple questions first: 1) In this patch the warning was disabled by default. But it sounds like you want it enabled by default? Easy to change, I'm just confirming your intent. 2) Is 'deprecated' handled differently than other types of warnings? There is a -Wno-deprecated, but it seems to have a very specific meaning that does not apply here. 3) The warning text in the old patch was "asm statement in function does not use extended syntax". The intent was: a) Don't make it sound like basic asm is completely gone, since it can still be used at top level. b) Don't make it sound like all inline asm is gone, since extended asm can still be used in functions. c) Convey all that in as few words as possible. Now that we want to add the word 'deprecated,' perhaps one of these: - Basic asm in functions is deprecated in favor of extended syntax - asm in functions without extended syntax is deprecated - Deprecated: basic asm in function - Deprecated: asm in function without extended syntax I like the last one (people may not know what 'basic' means in this context), but any of these would work for me. Preferences? In order to avoid conflicts, I'll wait for Bernd to commit his patch first. dw
Re: [PATCH] Make basic asm implicitly clobber memory
On 05/22/2016 04:33 AM, Andrew Haley wrote: On 05/20/2016 07:50 AM, David Wohlferd wrote: At a minimum, suddenly forcing an unexpected/unneeded memory clobber can adversely impact the optimization of surrounding code. This can be particularly annoying if the reason for the asm was to improve performance. And adding a memory clobber does add a dependency of sorts, which might cause the location of the asm to shift in an unfortunate way. And there's always the long-shot possibility that some weird quirk or (very) badly-written code will cause the asm to flat out fail when used with a memory clobber. And if this change does produce any of these problems, I feel pity for whoever has to track it down. OTOH, if a memory clobber does change code gen it probably changes it in a way which better fits user expectations, and perhaps it fixes a bug. That's a win, and it is far, far more important than any other consideration. My thoughts precisely. I realize deprecation/removal is drastic. Especially since basic asm (mostly) works as is. But fixing memory clobbers while leaving the rest broken feels like half a solution, meaning that some day we're going to have to fiddle with this again. Yes, we will undoubtedly have to fiddle with basic asm again. We should plan for deprecation. Right. There are some fundamental problems with basic asms and I think we want to deprecate them in the long term. In the immediate/medium term, I think addressing the memory dependency issue is the right thing to do. While it may make some code somewhere less optimized, it brings the basic asm semantics closer to what most programmers expect and prevents them from suddenly breaking as the optimizers continue to improve. If someone wants better optimized code, they ought to be using extended asms anyway. Jeff
Re: [PATCH] Make basic asm implicitly clobber memory
On Sun, 22 May 2016, Andrew Haley wrote: > On 05/20/2016 07:50 AM, David Wohlferd wrote: > > > At a minimum, suddenly forcing an unexpected/unneeded memory clobber > > can adversely impact the optimization of surrounding code. This can > > be particularly annoying if the reason for the asm was to improve > > performance. And adding a memory clobber does add a dependency of > > sorts, which might cause the location of the asm to shift in an > > unfortunate way. And there's always the long-shot possibility that > > some weird quirk or (very) badly-written code will cause the asm to > > flat out fail when used with a memory clobber. And if this change > > does produce any of these problems, I feel pity for whoever has to > > track it down. > > OTOH, if a memory clobber does change code gen it probably changes it > in a way which better fits user expectations, and perhaps it fixes a > bug. That's a win, and it is far, far more important than any other > consideration. > > Given that a basic asm statements has neither inputs nor outputs, it > must have side effects to be useful. All this patch does is recognize > that fact. I'm not saying your scenario won't occur, but it won't in > the majority of cases. > > > I realize deprecation/removal is drastic. Especially since basic > > asm (mostly) works as is. But fixing memory clobbers while leaving > > the rest broken feels like half a solution, meaning that some day > > we're going to have to fiddle with this again. > > Yes, we will undoubtedly have to fiddle with basic asm again. We > should plan for deprecation. > > But I think you're close to the all-or-nothing fallacy: that because > this patch doesn't solve all the problems with basic asm, it isn't > worth having. I think adding memory clobbers is worth having. I also think that deprecating basic asms would be a good thing, so can we please add a new warning for that? "warning: basic asms are deprecated" Richard.
Re: [PATCH] Make basic asm implicitly clobber memory
On 05/20/2016 07:50 AM, David Wohlferd wrote: > At a minimum, suddenly forcing an unexpected/unneeded memory clobber > can adversely impact the optimization of surrounding code. This can > be particularly annoying if the reason for the asm was to improve > performance. And adding a memory clobber does add a dependency of > sorts, which might cause the location of the asm to shift in an > unfortunate way. And there's always the long-shot possibility that > some weird quirk or (very) badly-written code will cause the asm to > flat out fail when used with a memory clobber. And if this change > does produce any of these problems, I feel pity for whoever has to > track it down. OTOH, if a memory clobber does change code gen it probably changes it in a way which better fits user expectations, and perhaps it fixes a bug. That's a win, and it is far, far more important than any other consideration. Given that a basic asm statements has neither inputs nor outputs, it must have side effects to be useful. All this patch does is recognize that fact. I'm not saying your scenario won't occur, but it won't in the majority of cases. > I realize deprecation/removal is drastic. Especially since basic > asm (mostly) works as is. But fixing memory clobbers while leaving > the rest broken feels like half a solution, meaning that some day > we're going to have to fiddle with this again. Yes, we will undoubtedly have to fiddle with basic asm again. We should plan for deprecation. But I think you're close to the all-or-nothing fallacy: that because this patch doesn't solve all the problems with basic asm, it isn't worth having. Andrew.
Re: [PATCH] Make basic asm implicitly clobber memory
On 5/18/2016 3:07 PM, Jeff Law wrote: On 05/07/2016 11:38 AM, Andrew Haley wrote: My argument in support of Bernd's proposal is that it makes sense from a *practical* software reliability point of view. It wouldn't hurt, and might fix some significant bugs. It's similar to the targets which always implicitly clobber "cc". It corresponds to what I always assumed basic asm did, and I'm sure that I'm not alone. This change might fix some real bugs and it is extremely unlikely to break anything. And by making basic asms use/clobber memory in particular, it means code using them is less likely to break as the optimizers continue to get smarter about memory loads/stores. I haven't gone through the actual patch yet, but I like it's basic goals. Other than the doc changes I already mentioned, I have no technical issues with the patch (although I don't speak 'internals' well enough to approve it either). That said, I'd like to make one final pitch for the alternate solution proposed by Richard Henderson: "deprecate and later completely remove basic asm within functions" IOW: Basic asm can only be used at "top level." Within functions, extended asm must be used. The reason I asked for clarification on what problem this patch is intended to solve, is that basic asm has a number of them. Given it: - Doesn't clobber memory - Doesn't clobber used registers - Doesn't clobber flags - Doesn't have dependencies This makes gcc's basic asm: - incompatible with user expectations. - incompatible with other compilers. - difficult for optimizers to correctly and consistently position. Bernd's patch adding the memory clobber does help with all of these. And I agree that it is unlikely to cause the generation of incorrect code. However unlike Andrew, I'm not prepared to say it doesn't 'hurt.' At a minimum, suddenly forcing an unexpected/unneeded memory clobber can adversely impact the optimization of surrounding code. This can be particularly annoying if the reason for the asm was to improve performance. And adding a memory clobber does add a dependency of sorts, which might cause the location of the asm to shift in an unfortunate way. And there's always the long-shot possibility that some weird quirk or (very) badly-written code will cause the asm to flat out fail when used with a memory clobber. And if this change does produce any of these problems, I feel pity for whoever has to track it down. But the real problem I have with this patch is that it doesn't actually solve the problem of user expectations. Or compatibility with other compilers. Or the problem with positioning. In fact this change makes the problem with expectations worse for the people who are knowledgeable about the current decades-old behavior. Ultimately, the design of gcc makes these problems inherently unsolvable. That's why I believe deprecation/removal is the real solution here. I realize deprecation/removal is drastic. Especially since basic asm (mostly) works as is. But fixing memory clobbers while leaving the rest broken feels like half a solution, meaning that some day we're going to have to fiddle with this again. If we are going to spend the time and effort (both ours and our users') to address basic-asm-in-a-function's problems, let's make sure we really solve them. dw
Re: [PATCH] Make basic asm implicitly clobber memory
On 05/07/2016 11:38 AM, Andrew Haley wrote: On 06/05/16 07:35, David Wohlferd wrote: 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." A few more things: Jeff Law did propose this, but it's impossible to do because it inevitably causes reload failures. Right. My argument in support of Bernd's proposal is that it makes sense from a *practical* software reliability point of view. It wouldn't hurt, and might fix some significant bugs. It's similar to the targets which always implicitly clobber "cc". It corresponds to what I always assumed basic asm did, and I'm sure that I'm not alone. This change might fix some real bugs and it is extremely unlikely to break anything. And by making basic asms use/clobber memory in particular, it means code using them is less likely to break as the optimizers continue to get smarter about memory loads/stores. I haven't gone through the actual patch yet, but I like it's basic goals. Jeff
Re: [PATCH] Make basic asm implicitly clobber memory
On 05/09/16 15:46, Bernd Schmidt wrote:
> On 05/09/2016 03:37 PM, Bernd Edlinger wrote:
>> On 05/09/16 09:56, Richard Biener wrote:
>>>
>>> At least it sounds to me that its semantics can be fully expressed
>>> with generic asms? (Maybe apart from the only-if-ASM_STRING-is-empty
>>> part)
>>>
>>
>> That was also my first idea too.
>>
>> In simple cases an asm ("whatever"); should do the same as
>> asm ("whatever" ::: );
>>
>> Adding a "memory" to the clobber list would be simple that's true.
>>
>> But in general it can be pretty complicated, especially if the
>> string contains the special characters % { | }.
>
> Is the only difference in how the string is output? Maybe we can have a
> slightly different form of ASM_OPERANDS (with a bit set, or with the
> string wrapped in something else) to indicate that it's old-style.
Most of the difference is what happens in final.c, and adding a new
attribute to the ASM_OPERANDS tree node is definitely one option.
I tried to implement it in a way that causes the least confusion.
There are lots of different tree representations for an extended asm
statement in genereal, but only one for a basic asm.
An extended asm that has no outputs and no clobbers, is an ASM_OPERAND
node with optional vector of ASM_INPUTs containig the input constraint:
ASM_OPERAND { "asm", "", 0, VEC { inputs...}, VEC { ASM_INPUT ("x")...}
but if it has any CLOBBERS, it will look like this:
PARALLEL { ASM_OPERAND, CLOBBER... }
if it has one output, and zero clobbers we have:
SET { x, ASM_OPERAND }
and in case we have more than one output we have:
PARALLEL { SET { x, ASM_OPERAND }... , CLOBBER... }
A basic asm is just an ASM_INPUT that is not underneath an ASM_OPERAND.
But to add any CLOBBERs to this ASM_INPUT it has to be in PARALLEL
with the CLOBBERs, so that would look like this:
PARALLEL { ASM_INPUT{ "asm" }, CLOBBER... }
There are lots of places where we need to know if a statement is an
assembler statement, in most places this is done in this way:
GET_CODE (PATTERN (insn)) == ASM_INPUT
|| asm_noperands (PATTERN (insn)) >= 0
There are a handful of places where it is done it this way:
GET_CODE (PATTERN (insn)) == ASM_INPUT
|| extract_asm_operands (PATTERN (insn)) != NULL_RTX
extract_asm_operands locates the ASM_OPERAND node from an extended
asm that can have either of the several forms above, but in most
cases the result is not looked at. Making extract_asm_operands
return anything but an ASM_OPERANDS is impossible, but making
asm_noperands return 0 for a PARALLEL { ASM_INPUT, CLOBBER... }
is not too complicated.
Fortunately, all the remaining uses of extract_asm_operands really
mean an extended asm.
Hope that explains my idea.
Thanks
Bernd.
Re: [PATCH] Make basic asm implicitly clobber memory
On 05/09/2016 03:37 PM, Bernd Edlinger wrote:
On 05/09/16 09:56, Richard Biener wrote:
At least it sounds to me that its semantics can be fully expressed
with generic asms? (Maybe apart from the only-if-ASM_STRING-is-empty
part)
That was also my first idea too.
In simple cases an asm ("whatever"); should do the same as
asm ("whatever" ::: );
Adding a "memory" to the clobber list would be simple that's true.
But in general it can be pretty complicated, especially if the
string contains the special characters % { | }.
Is the only difference in how the string is output? Maybe we can have a
slightly different form of ASM_OPERANDS (with a bit set, or with the
string wrapped in something else) to indicate that it's old-style.
Bernd
Re: [PATCH] Make basic asm implicitly clobber memory
On 05/09/16 09:56, Richard Biener wrote:
> On Thu, 5 May 2016, 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?
>
> I'm generally sympathetic with the change but I wonder if it would
> make sense to re-write "basic asm" into general asms to not
> need to special case them. I'd do that during gimplification
> for example.
>
> At least it sounds to me that its semantics can be fully expressed
> with generic asms? (Maybe apart from the only-if-ASM_STRING-is-empty
> part)
>
That was also my first idea too.
In simple cases an asm ("whatever"); should do the same as
asm ("whatever" ::: );
Adding a "memory" to the clobber list would be simple that's true.
But in general it can be pretty complicated, especially if the
string contains the special characters % { | }.
For example asm ("#%") is OK, but asm ("#%" :) is an ERROR.
So, single % must be duplicated, that may be a general rule (hopefully).
On some targets { | } mean different things, and must be escaped,
but on other targets these must not be escaped. So that is target
dependent.
Some targets replace whatever they want with the ASM_OUTPUT_OPCODE hook.
Example: i386 replaces "%v" with "v" or "", tilegx replaces "pseudo"
with "", this hook is only called with extended asm. And we must know
what it does and reverse it's effect. Here it starts to become
difficult.
And the ia64 target have different semantics for basic asm than without,
i.e. they always emit stop bits for traditional asms. So they
make a difference between extended and basic asm.
I have really no idea how to do this when extended and basic asm have
exactly the same tree structure.
That's why I thought I can as well add the clobbers to the basic asm's
tree representation.
Thanks
Bernd.
> Thanks,
> Richard.
>
Re: [PATCH] Make basic asm implicitly clobber memory
On Thu, 5 May 2016, 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? I'm generally sympathetic with the change but I wonder if it would make sense to re-write "basic asm" into general asms to not need to special case them. I'd do that during gimplification for example. At least it sounds to me that its semantics can be fully expressed with generic asms? (Maybe apart from the only-if-ASM_STRING-is-empty part) Thanks, Richard.
Re: [PATCH] Make basic asm implicitly clobber memory
On 06/05/16 07:35, David Wohlferd wrote: > 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." A few more things: Jeff Law did propose this, but it's impossible to do because it inevitably causes reload failures. My argument in support of Bernd's proposal is that it makes sense from a *practical* software reliability point of view. It wouldn't hurt, and might fix some significant bugs. It's similar to the targets which always implicitly clobber "cc". It corresponds to what I always assumed basic asm did, and I'm sure that I'm not alone. This change might fix some real bugs and it is extremely unlikely to break anything. Andrew.
Re: [PATCH] Make basic asm implicitly clobber memory
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
Re: [PATCH] Make basic asm implicitly clobber memory
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?
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?
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
Re: [PATCH] Make basic asm implicitly clobber memory
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
Re: [PATCH] Make basic asm implicitly clobber memory
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? 2) The -Wbasic-asm warning patch wasn't approved for v6. If we are going to change this behavior now, is it time? 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? 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." dw
Re: [PATCH] Make basic asm implicitly clobber memory, pr24414
On 12/09/2015 03:18 AM, Bernd Edlinger wrote:
Furthermore there is a documented use for asm(""): The empty assembler string
is used to make a function
volatile, thus calls can not be optimized away. But I think it is not
necessary to make this clobber anything,
nor should it be an instruction scheduling barrier, as it used to be in the
past.
Making that change seems risky; best not to make assumptions about how
these are used. In any case,
... or should I wait for the next stage1?
I think that would be better.
Bernd
Re: [PATCH] Make basic asm implicitly clobber memory, pr24414
Hi,
On 09.12.2015 12:06 Bernd Schmidt wrote:
> On 12/09/2015 03:18 AM, Bernd Edlinger wrote:
>> Furthermore there is a documented use for asm(""): The empty
>> assembler string is used to make a function
>> volatile, thus calls can not be optimized away. But I think it is
>> not necessary to make this clobber anything,
>> nor should it be an instruction scheduling barrier, as it used to be
>> in the past.
>
> Making that change seems risky; best not to make assumptions about how
> these are used. In any case,
>
So would you agree on the general direction of the patch,
if I drop the hunk in sched-deps.c ?
Thanks
Bernd.
Re: [PATCH] Make basic asm implicitly clobber memory, pr24414
Hi,
On 09.12.2015 16:48 Bernd Schmidt wrote:
> On 12/09/2015 04:09 PM, Bernd Edlinger wrote:
>
>> So would you agree on the general direction of the patch,
>> if I drop the hunk in sched-deps.c ?
>
> I'm not sure there was any consensus in that other thread, but I think
> assuming that basic asms clobber memory and CC, can be justified. That
> certainly seems like a safer default. Ideally though I think it would
> be best if we could deprecate basic asms in functions, or at least
> warn about them in -Wall.
>
>
Well no, we did not get to a consensus on the warning issue.
My personal gut feeling on that warning is a bit mixed...
If we have a -Wall-enabled warning on asm("..."), people who know next
to nothing
about assembler will be encouraged to "fix" this warning in a part of
the code which
they probably do not understand at all. This frightens me a bit.
Because I know they will soon find out, that adding a few colons fixes
the warning, but
asm("...":::) is not any better IMHO.
For me, it is just very very unlikely that any piece of assembler really
clobbers nothing
and has no inputs and no outputs at the same time, even it it looks so
at first sight...
It is much more likely that someone forgot to fill in the clobber section.
So for me it would also be good to warn on asm("...":::) and require
that, if they want
to fix this warning, they must at least write something in the clobber
section, like asm ("...":::"nothing"); that would be a new clobber name
which can only
stand alone and, which can get stripped after the warning processing
took place in the FE.
So I think a warning should warn on something that is so unusual that it
is likely a bug.
Bernd.
Re: [PATCH] Make basic asm implicitly clobber memory, pr24414
On 12/09/2015 04:09 PM, Bernd Edlinger wrote: So would you agree on the general direction of the patch, if I drop the hunk in sched-deps.c ? I'm not sure there was any consensus in that other thread, but I think assuming that basic asms clobber memory and CC, can be justified. That certainly seems like a safer default. Ideally though I think it would be best if we could deprecate basic asms in functions, or at least warn about them in -Wall. Bernd
