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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1136-1137
   "expected non-wide string literal in '#pragma %0'">, InGroup<IgnoredPragmas>;
+def warn_pragma_expected_ascii_string : Warning<
+  "expected ascii string literal in '#pragma %0'">, InGroup<IgnoredPragmas>;
 // - Generic errors
----------------
We use `warn_pragma_expected_non_wide_string` for all the other MS pragmas, so I think we should use that one here. Then, if we really care to, we can make `warn_pragma_expected_non_wide_string` more generally about non-ASCII string literals in a follow up.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:991-994
+def err_pragma_expected_file_scope : Error<
+  "'#pragma %0' can only appear at file scope">;
+
+def err_pragma_alloc_text_c_linkage: Error<
----------------



================
Comment at: clang/lib/Parse/ParsePragma.cpp:1206
+
+  ExpectAndConsume(tok::r_paren, diag::warn_pragma_expected_rparen, PragmaName);
+  if (ExpectAndConsume(tok::eof, diag::warn_pragma_extra_tokens_at_eol,
----------------
Should we be returning `false` if this comes back `true`? I'm worried about the case where there's a missing right paren, we warn about the pragma being ignored, find the EOL token and then actually handle the pragma.


================
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;
----------------
aaron.ballman wrote:
> 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.)
Ugh, I'm sorry, but I steered you wrong with this comment. I didn't notice until now that all the other MS pragmas use `getCharByteWidth() != 1` rather than `isAscii()`. I think we should replicate that mistake here, and then in a follow-up we can fix all the MS pragmas at once so that they consistently handle that case. WDYT?


================
Comment at: clang/lib/Sema/SemaAttr.cpp:748
+        &Functions) {
+  if (!CurContext->getRedeclContext()->isFileContext()) {
+    Diag(PragmaLocation, diag::err_pragma_expected_file_scope) << "alloc_text";
----------------
Same question here about whether we need to get the redecl context or not.


================
Comment at: clang/test/Sema/pragma-ms-alloc-text.cpp:5
+#pragma alloc_text(a        // expected-warning {{expected ',' in '#pragma alloc_text'}}
+#pragma alloc_text(a, a     // expected-warning {{missing ')' after '#pragma alloc_text'}} expected-error {{use of undeclared a}}
+#pragma alloc_text(L"a", a) // expected-warning {{expected a string literal for the section name}}
----------------
This shows we have the parsing logic wrong -- we shouldn't get the use of undeclared identifier a because we shouldn't have gotten into Sema for this one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125011