[cfe-dev] handling of volatile qualifier

[Cédric Venet]

Hi,


> You can also remove:
>        // FIXME: Volatility.

I removed the one from EmitStoreThroughLValue, the other are all ObjC
related, so I let them.

> Some >80 column violations I think, including:

I don't see any? Some line have just 80cols but I don't think I overflow.


> That FIXME is wrong; the mxcsr register is per-thread, and there's no
> point to volatile/restrict qualifying the memory operand.

I was thinking about the mxcsr, but if it is saved between context change
then no problem (I never really did x86 assembler, only µC ones)

> There's nothing to fix here; __builtin_ia32_loadlps takes a pointer to
> an unqualified vector.  Same for the other x86 builtins.

Ok, didn't had time to search what these instruction do, so I just put fixme
for more knowledgeable than me to fix

> 
> @@ -547,12 +562,14 @@
>    }
> 
>    FieldDecl *Field = E->getMemberDecl();
> -  return EmitLValueForField(BaseValue, Field, isUnion);
> +  return EmitLValueForField(BaseValue, Field, isUnion,
> +                            BaseExpr->getType().getCVRQualifiers());
>  }
> 
> Almost, but not quite: for an arrow operator, you care about the
> qualifiers on the pointed-to struct, not the pointer.

Corrected and added a case to the test file.

> 
> What about something like "int a() {return (volatile int){10};}"?
> (Not that this is a useful case, but might as well be consistent.)

The idea was that a literal (compound or not) is a constant expression, and
so don't need to be marked volatile?

> 
> Yes, it can be volatile, although I can't imagine that being of any
> use (example: "volatile struct {int x;} a(void);").

Ok, done. Don't think it will ever be used.

> 
> @@ -418,7 +426,8 @@
>        // Initializers can't initialize unnamed fields, e.g. "int :
> 20;"
>        continue;
>      }
> -    LValue FieldLoc = CGF.EmitLValueForField(DestPtr, CurField,
> isUnion);
> +    // FIXME: Volatile?
> +    LValue FieldLoc = CGF.EmitLValueForField(DestPtr, CurField,
> isUnion,0);
>      if (CurInitVal < NumInitElements) {
>        // Store the initializer into the field
>        // This will probably have to get a bit smarter when we support
> 
> I don't see anything to fix here.  (Although volatile aggregates are
> going to need a thorough audit once we can reasonably support a
> volatile aggregate copy.)
> 

Since you told me not to bother to much with aggregate, I just put 0 and a
fixme.
Now I removed the fixme.

> Otherwise, this is looking good!  Thanks for putting the time into
> this.

It is not much, I hope to do more (when I find the time and the motivation
simultaneously :)).

> 
>   // even if the return value of this statement isn't used (like here)
>   // clang add a volatile load which can't be removed.
>   // is it the correct behavior?
> 
> No, it's not really correct; I put in a hacky fix for the somewhat
> more serious bug that the assignment expression was coming up with the
> wrong value.  I'll fix it properly at some point.

Ok, I removed the comment.

-- 
Cédric
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: cg_volatile.c
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080610/1ffb7325/attachment.c>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fourth_try_volatile.patch
Type: application/octet-stream
Size: 23409 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080610/1ffb7325/attachment.obj>