This is an archive of the discontinued LLVM Phabricator instance.

[MSVC] Add support for pragma alloc_text
ClosedPublic

Authored by steplong on May 5 2022, 7:15 AM.

Details

Summary

#pragma alloc_text is a MSVC pragma that names the code section where functions should be placed. It only
applies to functions with C linkage.

https://docs.microsoft.com/en-us/cpp/preprocessor/alloc-text?view=msvc-170

Diff Detail

Event Timeline

steplong created this revision.May 5 2022, 7:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2022, 7:15 AM
steplong requested review of this revision.May 5 2022, 7:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2022, 7:15 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
steplong updated this revision to Diff 427329.May 5 2022, 8:09 AM
steplong added reviewers: rnk, aaron.ballman, hans.

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

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.

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.)

clang/include/clang/Sema/Sema.h
727
10353

This could probably have some comments with it.

clang/lib/Parse/ParsePragma.cpp
1166–1171

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?

1182–1185

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.)

1188

ExpectAndConsume()

1211–1215

ExpectAndConsume()

1222

ExpectAndConsume()

clang/lib/Sema/SemaAttr.cpp
1117–1118

Do we have to treat main() as being special?

1127

I think you need to use isInExternCContext() instead (and add test coverage for extern "C" { void func(void); }).

steplong updated this revision to Diff 428536.May 10 2022, 5:03 PM
  • Added Sema tests
  • Added checking of c linkage
  • Changed some parsing logic in the parser handler e.g #pragma alloc_text(a, foo without the right paren is only a warning
aaron.ballman added inline comments.May 11 2022, 4:34 AM
clang/include/clang/Basic/DiagnosticParseKinds.td
1136–1137 ↗(On Diff #428536)

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.

clang/include/clang/Basic/DiagnosticSemaKinds.td
991–994
clang/lib/Parse/ParsePragma.cpp
1182–1185

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?

1212

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.

clang/lib/Sema/SemaAttr.cpp
748

Same question here about whether we need to get the redecl context or not.

clang/test/Sema/pragma-ms-alloc-text.cpp
6

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.

steplong added inline comments.May 11 2022, 6:48 AM
clang/test/Sema/pragma-ms-alloc-text.cpp
6

MSVC accepts it and only warns about the missing paren if the identifier is declared like on Line 14. That's why I decided to handle it that way. We can decide to reject it though

aaron.ballman added inline comments.May 11 2022, 9:34 AM
clang/test/Sema/pragma-ms-alloc-text.cpp
6

I think this is a case we'd probably rather reject -- if someone runs into a problem where they're relying on the MSVC behavior, we can always relax the diagnostic later.

steplong updated this revision to Diff 428727.May 11 2022, 11:47 AM
  • Reject #pragma alloc_text(a, a
  • On Line 1180, I didn't use ExpectAndConsume() there because I wanted to accept both identifiers and strings (i.e pragma alloc_text(a, foo) and pragma alloc_text("a", foo)
  • It looks like we need ->getRedeclContext() to accept
extern "C" {
void foo();
#pragma alloc_text(a, foo)
}
aaron.ballman accepted this revision.May 12 2022, 7:04 AM

LGTM aside from some minor commenting nits that you can fix when landing. Can you also add a release note for the new pragma support?

clang/include/clang/Sema/Sema.h
727

Looks like this one was missed.

10353

Same here.

This revision is now accepted and ready to land.May 12 2022, 7:04 AM
steplong updated this revision to Diff 429000.May 12 2022, 10:33 AM
  • Added description in release notes
  • Fixed up some comments in Sema.h

Will merge once build passes

steplong updated this revision to Diff 429261.May 13 2022, 9:07 AM
  • Rebased patch
steplong updated this revision to Diff 429320.May 13 2022, 12:02 PM
  • Clang-formatted. It didn't like the empty line before the bracket in clang/test/CodeGen/msvc_pragma_alloc_text.cpp
This revision was automatically updated to reflect the committed changes.