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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
