================
Comment at: include/clang/AST/Decl.h:312
@@ -310,1 +311,3 @@
LabelStmt *TheStmt;
+ SmallString<16> MSAsmName;
+ bool MSAsmNameResolved;
----------------
ehsan wrote:
> rnk wrote:
> > This will cause a memory leak for names > 16 chars. When the ASTContext is
> > destroyed, it doesn't actually call destructors of individual AST nodes, it
> > merely frees the memory. This should be a StringRef that points to memory
> > from 'new (Context, 1) char[Len]', probably.
> Oh, I didn't know that! What's this 'new (Contex, 1) char[Len]' trick? I'm
> not familiar with it, I don't think...
It's an operator new overload declared in ASTContext.h. Basically, it's
equivalent to 'new char[Len]', except it goes calls the 'void* operator
new(size_t Bytes, ASTContext &Context, size_t Alignment)' overload.
================
Comment at: lib/Sema/SemaStmt.cpp:2357-2358
@@ +2356,4 @@
+ if (TheDecl->isMSAsmLabel()) {
+ // TODO Move this diagnostic elsewhere so that we get to examine the goto
+ // statements after we've seen all of the body of the current scope.
+ StmtResult error(StmtError(Diag(GotoLoc, diag::err_goto_ms_asm_label)
----------------
ehsan wrote:
> rnk wrote:
> > Does it help if you issue the same diagnostic when processing 'foo:' in
> > this example?
> > void f() {
> > goto foo;
> > __asm {
> > foo:
> > nop
> > }
> > }
> >
> > After issuing the diagnostic, you can avoid the "undeclared label" knock-on
> > errors by pretending that you did in fact define the label.
> I cannot issue the diagnostic there because I will not have a GotoStmt
> pointer when processing `foo:`. That's really the main issue.
>
> What I would ideally do would be to not issue any diagnostics in
> ActOnGotoStmt, and wait until the scope is fully processed, then visit all of
> the GotoStmts in the scope, check their LabelDecls to see if they're asm
> labels and issue the diagnostic there. But I don't know where I would hook
> into in order to be able to visit all of the GotoStmts at the end of a scope.
Looks like we have JumpDiagnostics.cpp for this purpose... See if you can work
it in there?
================
Comment at: lib/Sema/SemaStmtAsm.cpp:534
@@ +533,3 @@
+ // name.
+ OS << llvm::format("__MSASMLABEL_.%" PRIu32 "__", counter++);
+ Label->setMSAsmLabel(OS.str());
----------------
ehsan wrote:
> rnk wrote:
> > We should be able to use the 'L' assembler prefix to suppress the symbol
> > table entry for the label. I'm happy as long as both 'nm' and 'dumpbin
> > /symbols' agree that there are no MSASMLABEL symbols in the object file. I
> > think it will with the code as written.
> >
> > I'd also try to get the user-written label in there, so that the -S output
> > is more intelligible, something like:
> > OS << "L__MSASMLABEL_" << counter++ << "__" << ExternalLabelName;
> >
> > Users can't make labels that start with 'L', so we should be safe from
> > collisions, we just need a unique counter to make sure we don't collide
> > with ourselves.
> I think you brought this issue up before. :) I am turning these names into
> local symbol names in http://reviews.llvm.org/D4587 by using
> getPrivateGlobalPrefix() when rewriting `AOK_Label`. I don't think that we
> need to store that prefix in the AST, do we?
>
> Your suggestion to add the user-written label in the name is a good one, it
> just needs me to change all of these tests again! ;) But I'll do it in the
> next iteration.
>
> On the issue of counter uniqueness, the current global counter obviously
> ensures that. If we have one Sema object per each compilation, then making
> it a Sema member won't change things, so we should be good there.
OK, doing this in LLVM MC seems great.
================
Comment at: test/Sema/ms-inline-asm.c:145
@@ +144,3 @@
+void t10() {
+// TODO: The diagnostic below has the wrong location, because we don't have a
better location in CheckPoppedLabel
+foo: // expected-error {{use of undeclared label 'foo'}}
----------------
ehsan wrote:
> Any ideas how I can fix this? Or is the current misplaced diagnostic
> acceptable?
Moving to JumpDiagnostics.cpp will probably fix it.
http://reviews.llvm.org/D4589
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
