================
Comment at: include/clang/AST/Decl.h:312
@@ -310,1 +311,3 @@
LabelStmt *TheStmt;
+ SmallString<16> MSAsmName;
+ bool MSAsmNameResolved;
----------------
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...
================
Comment at: lib/Parse/ParseStmtAsm.cpp:99-101
@@ +98,5 @@
+ bool Create) override {
+ const llvm::MemoryBuffer *LBuf =
+ LSM.getMemoryBuffer(LSM.FindBufferContainingLoc(Location));
+ unsigned Offset = Location.getPointer() - LBuf->getBufferStart();
+ SourceLocation Loc = translateLocation(Offset);
----------------
rnk wrote:
> I think you can sink this into translateLocation if you make it take a
> SourceMgr and SMLoc.
Yeah looks like it. Will do.
================
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)
----------------
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.
================
Comment at: lib/Sema/SemaStmtAsm.cpp:522
@@ +521,3 @@
+ bool AlwaysCreate) {
+ static uint32_t counter = 0;
+
----------------
rnk wrote:
> This should be a member variable of Sema, rather than a global.
Sure.
================
Comment at: lib/Sema/SemaStmtAsm.cpp:534
@@ +533,3 @@
+ // name.
+ OS << llvm::format("__MSASMLABEL_.%" PRIu32 "__", counter++);
+ Label->setMSAsmLabel(OS.str());
----------------
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.
================
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'}}
----------------
Any ideas how I can fix this? Or is the current misplaced diagnostic
acceptable?
================
Comment at: test/Sema/ms-inline-asm.c:148
@@ +147,2 @@
+ __asm mov eax, foo
+}
----------------
rnk wrote:
> Can you add a .cpp test case involving nested scopes, like:
>
> void f() {
> __asm some_label:
> struct A {
> static void g() {
> __asm jmp some_label ; This should jump forwards
> __asm some_label:
> __asm nop
> }
> };
> }
Certainly!
http://reviews.llvm.org/D4589
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
