[cfe-dev] handling of volatile qualifier

[Eli Friedman]

On Sun, Jun 8, 2008 at 5:11 AM, Cédric Venet <cedric.venet at laposte.net> wrote:
> Hi,
>
> Here is a more complete implementation. There is style a lot of fixme,
> mainly in ObjC and Builtin because I don't know enough about this to do the
> right thing. I didn't run the test since I don't think I can do it on
> windows (without cygwin). I don't really know how to do testcase, but
> attached is a file that I used for testing my change and the result.

Okay; comments follow.

>> Nice work.  Just a few minor issues: One, the white-space in argument
>> lists is a bit inconsistent.
>
> I consistently put a space after a comma except if I need one less char to
> fit in 80cols. Is this a problem?
> For the return to line, I don't really know how to indent the lines. If you
> could point me to things not to do, I could correct them.

Oh, it was to fit into 80cols?  Then it's okay, I guess.

@@ -644,6 +644,8 @@
     llvm::Type *PtrTy = llvm::PointerType::getUnqual(llvm::Type::Int8Ty);
     Value *One = llvm::ConstantInt::get(llvm::Type::Int32Ty, 1);
     Value *Tmp = Builder.CreateAlloca(llvm::Type::Int32Ty, One, "tmp");
+    // FIXME: in multi threaded application, this should be volatile?
+    //        It should also be restrict probably
     Builder.CreateStore(Ops[0], Tmp);
     return Builder.CreateCall(CGM.getIntrinsic(Intrinsic::x86_sse_ldmxcsr),
                               Builder.CreateBitCast(Tmp, PtrTy));

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

We do actually have issues relating to re-arranging floating-point
operations around the call, but that's not something we can really
address in clang, so there's no point to having a FIXME there.

@@ -778,6 +782,7 @@
     unsigned Index = BuiltinID == X86::BI__builtin_ia32_loadlps ? 0 : 1;
     llvm::Value *Idx = llvm::ConstantInt::get(llvm::Type::Int32Ty, Index);
     Ops[1] = Builder.CreateBitCast(Ops[1],
llvm::PointerType::getUnqual(EltTy));
+    // FIXME: Volatility
     Ops[1] = Builder.CreateLoad(Ops[1], "tmp");
     Ops[0] = Builder.CreateBitCast(Ops[0], VecTy, "cast");
     Ops[0] = Builder.CreateInsertElement(Ops[0], Ops[1], Idx, "loadps");

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

@@ -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.

@@ -598,7 +618,7 @@
   llvm::Value *DeclPtr = CreateTempAlloca(LTy, ".compoundliteral");

   const Expr* InitExpr = E->getInitializer();
-  LValue Result = LValue::MakeAddr(DeclPtr);
+  LValue Result = LValue::MakeAddr(DeclPtr,0);

   if (E->getType()->isComplexType()) {
     EmitComplexExprIntoAddr(InitExpr, DeclPtr, false);

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


@@ -640,7 +660,8 @@
 LValue CodeGenFunction::EmitCallExprLValue(const CallExpr *E) {
   // Can only get l-value for call expression returning aggregate type
   RValue RV = EmitCallExpr(E);
-  return LValue::MakeAddr(RV.getAggregateAddr());
+  // FIXME: can this be volatile?
+  return LValue::MakeAddr(RV.getAggregateAddr(),0);
 }

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

@@ -808,6 +808,6 @@
     llvm::InlineAsm::get(FTy, AsmString, Constraints,
                          S.isVolatile() || S.getNumOutputs() == 0);
   llvm::Value *Result = Builder.CreateCall(IA, Args.begin(), Args.end(), "");
-  if (ResultAddr)
+  if (ResultAddr) // FIXME: volatility
     Builder.CreateStore(Result, ResultAddr);
 }

Hmm, there isn't really any reasonable way to deal with volatile in an
asm statement... we should probably make sure we warn in Sema.  Just
leave the fixme for the moment, though.

@@ -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.)

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

I'll deal with turning the test into an automated test... although I'm
not completely sure how I'll do it; possibly I'll make it grep for the
correct number of volatile loads and stores.

  // 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.

-Eli