[PATCH] D125011: [MSVC] Add support for pragma alloc_text

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thanks for working on this compatibility extension!

This is missing all of the lexing tests which validate that we correctly diagnose malformed pragmas. We're also missing all sema tests for the diagnostics added or emitted from there. When you add the sema tests, I think we should have diagnostics for:

  // First case
  #pragma alloc_text("abc", does_not_exist)
  
  // Second case
  void already_defined(void) {}
  #pragma alloc_text("abc", already_defined)

Also, what should happen in a case like this?

  __declspec(code_seg("text")) static void section_mismatch(void);
  #pragma alloc_text("hoo boy", section_mismatch) // Pretty sure we want to diagnose this?

MSDN docs say that the pragma "can't be used in a function" which is subtly different from "at file scope". e.g., MSVC accepts:

  void umm(void);
  struct S {
  #pragma alloc_text("hoo boy", umm)
    int a;
  };

which is a bit of a silly example, but it also accepts:

  extern "C" void umm(void);
  namespace N {
  #pragma alloc_text("hoo boy", umm)
  }
  
  // or
  extern "C" {
   void okay(void);
  #pragma alloc_text("hoo boy", okay)
  }

which are far more reasonable for users to write. I think we should have tests for these situations to make sure we accept the same code as MSVC unless the behavior there seems like a bug.

In D125011#3494014 <https://reviews.llvm.org/D125011#3494014>, @steplong wrote:

> Update patch to error out if a function is not C linkage. Not sure how to check if a function has been declared

At the time we're processing the pragma (within Sema), I would perform a lookup on the identifier given by the pragma to validate that it 1) can be found, 2) finds a function, 3) the function found is in an extern "C" context. To do the lookup, I'd probably use `Sema::LookupSingleName()` because this can't be used on an overloaded function. (And you should add tests for specifying a member function and an overloaded function.)



================
Comment at: clang/include/clang/Sema/Sema.h:727
 
+  /// Sections used with #pragma alloc_text
+  llvm::StringMap<std::tuple<StringRef, SourceLocation>> FunctionToSectionMap;
----------------



================
Comment at: clang/include/clang/Sema/Sema.h:10337
 
+  void AddSectionMSAllocText(FunctionDecl *FD);
+
----------------
This could probably have some comments with it.


================
Comment at: clang/lib/Parse/ParsePragma.cpp:1160-1165
+  if (Tok.isNot(tok::l_paren)) {
+    PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_lparen)
+        << PragmaName;
+    return false;
+  }
+  PP.Lex(Tok);
----------------
This looks like it can be replaced with `ExpectAndConsume()`. However, should we be diagnosing use of this pragma for non-MSVC compatible triples before we start parsing?


================
Comment at: clang/lib/Parse/ParsePragma.cpp:1176-1179
+  if (SegmentName->getCharByteWidth() != 1) {
+    PP.Diag(PragmaLocation, diag::warn_pragma_expected_non_wide_string)
+        << PragmaName;
+    return false;
----------------
I think we want to test `isAscii()` instead of looking at the byte width; we don't want to accept `u8"whatever"` any more than we want to accept `L"whatever"`.

(The diagnostic text is also pretty bad because it'll diagnose non-wide literals as being wide, but that's an existing deficiency with the diagnostic and can be addressed separately.)


================
Comment at: clang/lib/Parse/ParsePragma.cpp:1182
+
+  if (Tok.isNot(tok::comma)) {
+    PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_comma) << PragmaName;
----------------
`ExpectAndConsume()`


================
Comment at: clang/lib/Parse/ParsePragma.cpp:1205-1209
+  if (Tok.isNot(tok::r_paren)) {
+    PP.Diag(PragmaLocation, diag::warn_pragma_expected_rparen) << PragmaName;
+    return false;
+  }
+  PP.Lex(Tok);
----------------
`ExpectAndConsume()`


================
Comment at: clang/lib/Parse/ParsePragma.cpp:1216
+  }
+  PP.Lex(Tok);
+
----------------
`ExpectAndConsume()`


================
Comment at: clang/lib/Sema/SemaAttr.cpp:1094-1095
+void Sema::AddSectionMSAllocText(FunctionDecl *FD) {
+  if (!FD->getIdentifier())
+    return;
+
----------------
Do we have to treat `main()` as being special?


================
Comment at: clang/lib/Sema/SemaAttr.cpp:1104
+
+    if (!FD->isExternC()) {
+      Diag(Loc, diag::err_pragma_alloc_text_c_linkage);
----------------
I think you need to use `isInExternCContext()` instead (and add test coverage for `extern "C" { void func(void); }`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125011