aaron.ballman added inline comments.
================
Comment at: clang/lib/Sema/SemaAttr.cpp:1070
+ SourceLocation Loc, const llvm::SmallSetVector<StringRef, 4> &Intrinsics) {
+ if (!CurContext->getRedeclContext()->isFileContext()) {
+ Diag(Loc, diag::err_pragma_intrinsic_function_scope);
----------------
steplong wrote:
> aaron.ballman wrote:
> > steplong wrote:
> > > aaron.ballman wrote:
> > > > steplong wrote:
> > > > > aaron.ballman wrote:
> > > > > > What is `CurContext` when this gets called for your .c test file? I
> > > > > > would have expected it to be the `TranslationUnitDecl` which should
> > > > > > be a file context (getting the redecl context shouldn't do anything
> > > > > > in the cases I've seen).
> > > > > It looks like it's a `FunctionDecl`
> > > > Wha? That seems rather surprising -- which function does it think the
> > > > pragma is scoped to?
> > > >
> > > > You might consider putting breakpoints in `PushDeclContext()` and
> > > > `PopDeclContext()` to see what's going on (or search for other places
> > > > where we assign to `CurContext` and break on those).
> > > This is what I see when I run it on pragma-ms-function.c:
> > > ```
> > > PUSHING TranslationUnit
> > > PUSHING Function foo1
> > > FOUND PRAGMA FUNCTION
> > > POPPING Function foo1
> > > PUSHING TranslationUnit
> > > PUSHING Function foo2
> > > FOUND PRAGMA FUNCTION
> > > POPPING Function foo2
> > > PUSHING TranslationUnit
> > > PUSHING Function foo3
> > > POPPING Function foo3
> > > PUSHING TranslationUnit
> > > ```
> > > I'm logging the swap in `PopDeclContext()` with POPPING and PUSHING and
> > > the push in `PushDeclContext()` with just PUSHING.
> > > I'm also logging FOUND PRAGMA in the pragma handler
> > Huh. For fun, can you try changing the test to:
> > ```
> > void foo1(char *s, char *d, size_t n) {
> > bar(s);
> > memset(s, 0, n);
> > memcpy(d, s, n);
> > }
> >
> > int i; // Now there's a declaration here
> >
> > #pragma function(strlen, memset)
> > ```
> > and see if you get different results? I'm wondering if what's happening is
> > that the `CurContext` is being updated after we've lexed the next token
> > from the preprocessor, which means we're still in the context of `foo1`
> > until after we've processed the pragma due to it being a preprocessing
> > token. It still wouldn't make much sense to me, because I think we should
> > hit that on the `}` for `foo1()`, but it's a shot in the dark.
> It looks like you're right. The `int i` makes the pragma show up after the
> `TranslationUnit` is pushed.
>
> ```
> PUSHING TranslationUnit
> PUSHING Function foo1
> POPPING Function foo1
> PUSHING TranslationUnit
> FOUND PRAGMA FUNCTION
> PUSHING Function foo2
> POPPING Function foo2
> PUSHING TranslationUnit
> FOUND PRAGMA FUNCTION
> PUSHING Function foo3
> POPPING Function foo3
> PUSHING TranslationUnit
> ```
>
> I really appreciate the review and you helping me debug this
Ugggghhhhh I was hoping I was wrong because I think the solution requires some
experimentation that may or may not pan out.
The way the pragma is currently being handled is: we parse everything at
preprocessing time (`PragmaMSFunctionHandler::HandlePragma()`) and then call
directly into Sema (`Actions.ActOnPragmaMSFunction()`).
The way I think we may have to handle the pragma is: identify the pragma at
preprocessing time, emit an annotated token stream, parse that token stream
from the parser, and then call Sema from there. e.g., more like:
https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParsePragma.cpp#L2698
https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/Parser.cpp#L830
https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParsePragma.cpp#L894
(you would call `ActOnPragmaMSFunction()` from here)
This way, the appropriate parsing contexts are all updated (hopefully)!
> I really appreciate the review and you helping me debug this
I'm very happy to help!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124702/new/
https://reviews.llvm.org/D124702
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
