[PATCH] D124702: [MSVC] Add support for pragma function

[Aaron Ballman via Phabricator via cfe-commits]

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