This is an archive of the discontinued LLVM Phabricator instance.

[clang-cl] Handle some pragma alloc_text corner cases handled by MSVC
ClosedPublic

Authored by steplong on Jun 27 2022, 7:38 AM.

Details

Summary

MSVC's pragma alloc_text accepts a function that was redeclared in
a non extern-C context if the previous declaration was in an extern-C
context. i.e.

extern "C" { static void f(); }
static void f();

MSVC's pragma alloc_text also rejects non-functions.

Diff Detail

Event Timeline

steplong created this revision.Jun 27 2022, 7:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2022, 7:38 AM
steplong requested review of this revision.Jun 27 2022, 7:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2022, 7:38 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This does make us accept, but MSVC doesn't:

static void f();
extern "C" {
  static void f();
}
#pragma alloc_text(c, f);
static void f() {}

MSVC and GCC fail with an error for conflicting declaration.
GCC: https://godbolt.org/z/Tavvx88E4
MSVC: https://godbolt.org/z/vj41TWdYh

hans added inline comments.Jun 27 2022, 8:08 AM
clang/lib/Sema/SemaAttr.cpp
811

Since the pragma only applies to functions, maybe we should error here if the decl is not a FunctionDecl?

816

Instead of walking the decls, would checking isExternC() on the getCanonicalDecl() be enough?

steplong updated this revision to Diff 440268.Jun 27 2022, 9:31 AM
  • Use getCanonicalDecl(). This does make us reject (MSVC rejects it already because of conflicting declarations):
static void f();
extern "C" { static void f(); }
static void f() {}
  • Error if not function decl

I think we have to use isInExternCContext() to accept the following (MSVC accepts this):

extern "C" { static void f(); }
static void f() {}
steplong retitled this revision from [clang-cl] Accept a pragma alloc_text corner case accepted by MSVC to [clang-cl] Handle some pragma alloc_text corner cases handled by MSVC.Jun 27 2022, 9:35 AM
steplong edited the summary of this revision. (Show Details)
hans added a comment.Jun 27 2022, 9:48 AM

I think we have to use isInExternCContext() to accept the following (MSVC accepts this):

extern "C" { static void f(); }
static void f() {}

I'm probably missing something. Is the "static" important in this example? Since f has internal linkage, I don't see why a user would care if it's "extern C" or not, and also not in which section it would go (in this case it will not be emitted at all, because it's not used).

Isn't the question whether f is considered "extern C" in the end or not? I thought isExternC() checks that? Are you saying it would return false for f in your example?

clang/lib/Sema/SemaAttr.cpp
822

Since it's inside if (getLangOpts().CPlusPlus) { it will only error in C++ mode, but it should probably also error in C mode.

Isn't the question whether f is considered "extern C" in the end or not? I thought isExternC() checks that? Are you saying it would return false for f in your example?

Yup, isExternC() is returning false for that case because there's "static".

I'm probably missing something. Is the "static" important in this example? Since f has internal linkage, I don't see why a user would care if it's "extern C" or not, and also not in which section it would go (in this case it will not be emitted at all, because it's not used).

Hmm, good point. This is the disassembly I see for:

extern "C" { static void f(); }
static void f();
#pragma alloc_text(s, f)
static void f() {}

void bar() {
  f();
}

Disassembly of section .text$mn:
?bar@@YAXXZ:
       0:       fd 7b bf a9     stp     x29, x30, [sp, #-16]!
       4:       fd 03 00 91     mov     x29, sp
       8:       00 00 00 94     bl      #0 <?bar@@YAXXZ+0x8>
       c:       fd 7b c1 a8     ldp     x29, x30, [sp], #16
      10:       c0 03 5f d6     ret
Disassembly of section s:
s:
       0:       c0 03 5f d6     ret
steplong updated this revision to Diff 440344.Jun 27 2022, 11:54 AM
  • Error for c also
hans added a comment.Jun 28 2022, 1:53 AM

Isn't the question whether f is considered "extern C" in the end or not? I thought isExternC() checks that? Are you saying it would return false for f in your example?

Yup, isExternC() is returning false for that case because there's "static".

I see. It seems there's already some code that gives special treatment to "static extern c" functions in CodeGenModule::MaybeHandleStaticInExternC(): https://github.com/llvm/llvm-project/blob/llvmorg-14.0.6/clang/lib/CodeGen/CodeGenModule.cpp#L4420

Okay, I think this is almost ready. I just had one more comment about the if-statements above.

clang/lib/Sema/SemaAttr.cpp
824

Instead of checking this in two places, how about doing something like:

FunctionDecl *FD = dyn_cast<FunctionDecl>(ND->getCanonicalDecl());
if (!FD) {
  // error
}

above right after the if (!ND) check? Then the C++ specific code just becomes

if (!FD->isInExternCContext()) {
  // error
}
steplong updated this revision to Diff 440618.Jun 28 2022, 7:19 AM
  • Rework logic for rejecting non-functions and non-extern C functions when C++
hans accepted this revision.Jun 28 2022, 7:23 AM

lgtm

This revision is now accepted and ready to land.Jun 28 2022, 7:23 AM
This revision was landed with ongoing or failed builds.Jun 29 2022, 6:46 AM
This revision was automatically updated to reflect the committed changes.