jyu2 added inline comments.
================
Comment at: lib/AST/Stmt.cpp:457-460
this->NumOutputs = NumOutputs;
this->NumInputs = NumInputs;
this->NumClobbers = NumClobbers;
+ this->NumLabels = NumLabels;
----------------
rsmith wrote:
> Please assert that you don't have both outputs and labels here. (If you did,
> you would assign them the same slots within `Exprs`.)
>
> You also seem to be missing `Sema` enforcement of the rule that you cannot
> have both outputs and labels. (If you want to actually support that as an
> intentional extension to the GCC functionality, you should change the
> implementation of `GCCAsmStmt` to use different slots for outputs and labels,
> add some tests, and add a `-Wgcc-compat` warning for that case. Seems better
> to just add an error for it for now.)
This is enforcement during the parer.
for example:
a.c:12:23: error: expected ':'
asm goto ("decl %0;" :"a": "m"(cond) : "memory" );
Do you think this is enough for now?
Thanks.
Jennifer
================
Comment at: lib/Analysis/CFG.cpp:1445-1458
+ LabelMapTy::iterator LI = LabelMap.find(G->getLabel());;
+ // If there is no target for the goto, then we are looking at an
+ // incomplete AST. Handle this by not registering a successor.
+ if (LI == LabelMap.end()) continue;
- JumpTarget JT = LI->second;
- prependAutomaticObjLifetimeWithTerminator(B, I->scopePosition,
- JT.scopePosition);
- prependAutomaticObjDtorsWithTerminator(B, I->scopePosition,
- JT.scopePosition);
- const VarDecl *VD = prependAutomaticObjScopeEndWithTerminator(
- B, I->scopePosition, JT.scopePosition);
- appendScopeBegin(JT.block, VD, G);
- addSuccessor(B, JT.block);
+ JumpTarget JT = LI->second;
+ prependAutomaticObjLifetimeWithTerminator(B, I->scopePosition,
----------------
rsmith wrote:
> Please factor out this duplicated code into a local lambda rather than
> copy-pasting it.
Thanks. Changed.
================
Comment at: lib/Analysis/CFG.cpp:1461
+ if (auto *G = dyn_cast<GCCAsmStmt>(B->getTerminator())) {
+ if (G->isAsmGoto()) {
+ for (auto *E : G->labels()) {
----------------
rsmith wrote:
> This check is redundant; there will be no labels if it's not an `asm goto` so
> you can just perform the below loop unconditionally.
Okay removed. Thanks.
================
Comment at: lib/Analysis/CFG.cpp:3139-3141
+ // We will need to backpatch this block later.
+ BackpatchBlocks.push_back(JumpSource(Block, ScopePos));
+ return Block;
----------------
rsmith wrote:
> Do we not need the handling for the other labels if we hit this condition for
> one label?
It is handled in backpatch processing which go though each label and build CFG.
================
Comment at: lib/Sema/JumpDiagnostics.cpp:347
+ LabelAndGotoScopes[S] = ParentScope;
+ Jumps.push_back(S);
+ }
----------------
efriedma wrote:
> jyu2 wrote:
> > efriedma wrote:
> > > This doesn't look right; I think we need to add it to IndirectJumps
> > > instead. This probably impacts a testcase like the following:
> > >
> > > ```
> > > struct S { ~S(); };
> > > int f() {
> > > {
> > > S s;
> > > asm goto(""::::BAR);
> > > return 1;
> > > }
> > > BAR:
> > > return 0;
> > > }
> > > ```
> > >
> > > (gcc currently accepts this and skips running the destructor, but I'm
> > > pretty sure that's a bug.)
> > Hi Eli,
> > I see both g++ and clang++ with my change call ~S. Am I missing something?
> > Thanks.
> >
> > 1>clang++ j.cpp
> > 1>./a.out
> > ~S()
> > 1>g++ j.cpp
> > 1>./a.out
> > ~S()
> >
> > Here is the test case:
> > 1>cat j.cpp
> > extern "C" int printf (const char *,...);
> > struct S { ~S() {printf("~S()\n");}; };
> > int f() {
> > {
> > S s;
> > asm goto(""::::BAR);
> > return 1;
> > }
> > BAR:
> > return 0;
> > }
> >
> > int main()
> > {
> > f();
> > }
> >
> Oh, sorry, I wasn't paying attention to the actual contents of the asm. If
> you modify the asm goto slightly, to `asm goto("jmp %0"::::BAR);`, you'll see
> that the destructor doesn't run, at least with gcc. (This is different from
> the way `goto BAR;` works.)
Thanks. I see, the cleanup code is not generated for asm goto.
For clang with our current implementation, llvm emit error for this. But the
error message isn't clear.
You are right, we should emit error in clang. I will look into tomorrow.
Thanks.
================
Comment at: lib/Sema/SemaStmtAsm.cpp:656
+ // Check for duplicate asm operand name between input, output and label
lists.
+ typedef std::pair <StringRef , Expr *> MyItemType;
+ SmallVector<MyItemType, 4> MyList;
----------------
rsmith wrote:
> No space after `pair`, please.
Sorry, changed.
================
Comment at: lib/Sema/SemaStmtAsm.cpp:659-662
+ for (unsigned i = 0, e = NumOutputs + NumInputs + NumLabels; i != e; ++i)
+ if (NS->getIdentifier(i) && !NS->getIdentifier(i)->getName().empty())
+ MyList.emplace_back(std::make_pair(NS->getIdentifier(i)->getName(),
+ NS->getOperandExpr(i)));
----------------
rsmith wrote:
> Looping over all three lists of expressions here is effectively hardcoding an
> implementation detail of `GCCAsmStmt` into this code, violating its
> encapsulation. Instead, please write three loops (iterating over the inputs,
> outputs, and labels), and remove `getOperandExpr` entirely.
changed.
================
Comment at: lib/Sema/SemaStmtAsm.cpp:660
+ for (unsigned i = 0, e = NumOutputs + NumInputs + NumLabels; i != e; ++i)
+ if (NS->getIdentifier(i) && !NS->getIdentifier(i)->getName().empty())
+ MyList.emplace_back(std::make_pair(NS->getIdentifier(i)->getName(),
----------------
rsmith wrote:
> Checking whether the name is empty is redundant: we should never create an
> `IdentifierInfo` representing an empty string.
Changed. Thanks.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56571/new/
https://reviews.llvm.org/D56571
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
