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

[Stephen Long via Phabricator via cfe-commits]

steplong 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);
----------------
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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124702/new/

https://reviews.llvm.org/D124702