Re: basic asm and memory clobbers - Proposed solution

[David Wohlferd <dw at LimeGreenSocks dot com>]

On 12/18/2015 11:55 AM, Bernd Edlinger wrote:
> On 18.12.2015 10:27, David Wohlferd wrote:
> > On 12/17/2015 11:30 AM, Bernd Edlinger wrote:
> > > Adding this warning to -Wall is too quickly and will bring the ia64, 
> > > tilegx and mep ports into trouble. 
> > It doesn't look to me like adding the warnings will affect gcc
> > itself.  But I do see how it could have an impact on people using gcc
> > on those platforms, if the warning causes them to convert to extended
> > asm.
> At least we should not start a panic until we have really understood all
> the details, how to do that.

Phase 1 is a better place to start a panic.

> > > mep: mep_interrupt_saved_reg looks for ASM_INPUT in the body, and
> > > saves different registers if found.
> > I'm trying to follow this code.  A real challenge since I know nothing
> > about mep.  But what I see is:
> >
> > - This routine only applies to functions marked as
> > __attribute__((interrupt)).
> > - To correctly generate entry/exit, it walks thru each register (up to
> > FIRST_PSEUDO_REGISTER) to see if it is used by the routine.  If there
> > is any basic asm within the routine (regardless of its contents), the
> > register is considered 'in use.'
> >
> > The net result is that every register gets saved/restored by the
> > entry/exit of this routine if it contains basic asm.  The reason this
> > is important is that if someone just adds a colon, it would suddenly
> > *not* save/restore all the registers.  Depending on what the asm does,
> > this could be bad.
> >
> > Does that sound about right?
> Yes.

Seems like a doc update would be appropriate here too then, if anyone 
wanted to volunteer...

> > This is certainly worth mentioning in the 'convert' doc.
> >
> > I wonder how often this 'auto-clobber' feature is used, though.  I
> > don't see it mentioned in the 'interrupt' attribute docs for mep, and
> > it's not in the basic asm docs either.  If your interrupt doesn't need
> > many registers, it seems like you'd want to know this and possibly use
> > extended.  And you'd really want to know if you are doing a
> > (redundant) push/pop in your interrupt.
> >
> > > tilegx:  They never wrap {} around inline asm. But extended asm, are
> > > handled differently, probably automatically surrounded by braces.
> > I know nothing about tilegx either.  I've tried to read the code, and
> > it seems like basic asm does not get 'bundled' while extended might be.
> >
> > Bundling for tilegx (as I understand it) is when you explicitly fill
> > multiple pipelines by doing something like this:
> >
> >     { add r3,r4,r5 ; add r7,r8,r9 ; lw r10,r11 }
> >
> > So if you have a basic asm statement, you wouldn't want it
> > automatically bundled by the compiler, since your asm could be more
> > than 3 statements (the max?).  Or your asm may do its own bundling. So
> > it makes sense to never output braces when outputting basic asm.
> >
> > I know I'm guessing about what this means, but doesn't it seem like
> > those same limitations would apply to extended?  I wonder if this is a
> > bug.  I don't see any discussion of bundling (of this sort) in the docs.
> I wold like to build a cross compiler, but currently that target seems
> to be broken. I have to check
> that target anyways, because of my other patch with the memory clobbers.
>
> I see in tilegx_asm_output_opcode, that they do somehow automatically
> place braces.
> An asm("pseudo":) has a special meaning, and can be replaced with "" or "}".
> However the static buf[100] means that any extended asm string > 95
> characters, invokes
> the gcc_assert in line 5487.  In the moment I would _not_ recommend
> changing any asm
> statements without very careful testing.

Yes, the handling of extended asm there is very strange.

If bundles can only be 3 instructions, then appending an entire extended 
asm in a single (already in-use) bundle seems odd.  But maybe that's 
just because I don't understand tilegx.

I'm not sure it's just changing basic asm to extended I would be 
concerned about.  I'd worry about using extended asm at all...

> > > ia64:  ASM_INPUT emits stop-bits. extended asm does not the same.
> > > That was already mentioned by Segher.
> > I already had this one from Segher's email.
> >
> > --------------------
> > Given all this, I'm more convinced than ever of the value of
> > -Wonly-top-basic-asm.  Basic asm is quirky, has undocumented bits and
> > its behavior is incompatible with other compilers, even though it uses
> > the same syntax.  If I had any of this in my projects, I'd sure want
> > to find it and look it over.
> >
> > But maybe Bernd is right and it's best to leave the warning disabled
> > in v6, even by -Wall.  I may ask this question again in the next phase
> > 1...
> Aehm, yes, maybe the warning could by then be something more reasonable
> like:
> "Warning: the semantic of basic asm has changed to include implicit
> memory clobber,
> if you think that is a problem for you, please convert it to basic asm,
> otherwise just relax."

I don't think Jeff wants to pursue changes to basic asm's semantics as 
part of v6.  I can't blame him, it's a big change for phase 3. So it 
seems a bit early for the warning to talk about changes in semantics.

> > With that in mind, how do you feel about the basic plan:
> >
> > - Update the basic asm docs to describe basic asm's current (and
> > historical) semantics (ie clobber nothing).
> > - Emphasize how that might be different from users' expectations or
> > the behavior of other compilers.
> > - Warn that this could change in future versions of gcc.  To avoid
> > impacts from this change, use extended.
> > - Reference the "How to convert from basic asm to extended asm" guide
> > (where ever it ends up living).
> > - Mention -Wonly-top-basic-asm as a way to locate affected statements.
> > - -Wonly-top-basic-asm is disabled by default and not enabled by -Wall
> > or -Wextra.
> >
> > Does this seem like a safe and useful change for v6?
> Yes, but please add a test case with some examples where the warning is
> expected to be triggered,
> and where it is not.

I have one, but I haven't added the dejagnu stuff to it yet.  I'm not 
really an expert there and could use some help.  Simple things like 
where does the file go and what should it be called.

dw