Page MenuHomePhabricator

Implement #pragma clang restrict_expansion
ClosedPublic

Authored by beanz on Jul 29 2021, 11:39 AM.

Details

Summary

This patch adds #pragma clang restrict_expansion to enable flagging
macros as unsafe for header use. This is to allow macros that may have
ABI implications to be avoided in headers that have ABI stability
promises.

Using macros in headers (particularly public headers) can cause a
variety of issues relating to ABI and modules. This new pragma logs
warnings when using annotated macros outside the main source file.

This warning is added under a new diagnostics group -Wpedantic-macros

Diff Detail

Event Timeline

beanz created this revision.Jul 29 2021, 11:39 AM
beanz requested review of this revision.Jul 29 2021, 11:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 29 2021, 11:39 AM
beanz updated this revision to Diff 362840.Jul 29 2021, 11:40 AM

Fixing an inadvertant change.

beanz retitled this revision from This patch adds `#pragma clang header_unsafe` to enable flagging macros as unsafe for header use. This is to allow macros that may have ABI implications to be avoided in headers that have ABI stability promises. to Implement #pragma clang header_unsafe.Jul 29 2021, 11:51 AM
beanz edited the summary of this revision. (Show Details)
beanz updated this revision to Diff 362856.Jul 29 2021, 1:06 PM

Re-adding a newline that was inadvertently remvoed.

beanz added a subscriber: lattner.

+@lattner,

Chris provided some post-commit feedback on the macro deprecation patch, I'll address his feedback here since I'm touching the same code.

Chris' feedback is:

/clang/lib/Lex/Preprocessor.cpp:1417 Random drive-by, but in non-LTO builds of clang, this is adding an unnecessary call/return on the expansion path for all non-deprecated macros (the common case). I'd recommend something like:

// Inline in header:
void emitMacroExpansionWarnings(const Token &Identifier) {
if (Identifier.getIdentifierInfo()->isDeprecatedMacro())

emitMacroExpansionWarningsImpl(Identifier);

}

and then put emitMacroExpansionWarningsImpl in the .cpp file.

Thanks!

beanz updated this revision to Diff 363550.Aug 2 2021, 12:52 PM

Updates based on post-commit feedback from @lattner on D106732.

This should inline the determination for emitting warnings, but keep the actual
warning emission in a call.

aaron.ballman added inline comments.Aug 9 2021, 11:28 AM
clang/docs/LanguageExtensions.rst
3938

I think it should be made more clear as to whether this "unsafe to use in headers" means "unsafe to use in the header declaring the attribute as unsafe" vs "unsafe to use in any header included by the one marked as unsafe.", etc.

There's also the rather interesting example of:

// foo.h
#define FROBBLE 1
#pragma clang header_unsafe(FROBBLE, "why would you ever frobble things?")

// bar.h
#if 1
#elifdef FROBBLE
#endif

// source.c
#include "foo.h"
#include "bar.h"

Do we diagnose use of FROBBLE in bar.h despite it not directly including foo.h? Do we diagnose even though it's in a branch that's not possible to take? Does/should the answer change if we swap the order of includes? What if we change #if 1 into something that's actually variable like FOO > 8?

clang/include/clang/Basic/DiagnosticGroups.td
1316

Speaking of making things pedantic... :-D

clang/include/clang/Basic/IdentifierTable.h
127–130
192–193
clang/include/clang/Lex/Preprocessor.h
804
2424

Is this going to cause a copy for Msg without a call to move()?

clang/lib/Lex/Preprocessor.cpp
1416–1435

I think it'd make sense to have a "macro marked <whatever> here" note in both of these pragmas (the deprecated one can be handled separately as a later thing), WDYT?

clang/test/Lexer/Inputs/unsafe-macro-2.h
24–27

Why do we not expect warnings for these cases? I would have expected that undefining a macro is just as unsafe for ABI reasons as defining a macro is.

beanz marked 5 inline comments as done.Aug 9 2021, 4:03 PM

@aaron.ballman, thanks for the feedback!

Some comments, but I'll work on updated patches and try to get them up tonight or tomorrow morning.

clang/docs/LanguageExtensions.rst
3938

My intent was that once marked unsafe, it would warn for _all_ uses in files that aren't the main source file.

In your example, the warning wouldn't fire because the preprocessor doesn't expand skipped block conditionals, but it would if the block wasn't skipped.

It also is order-based, so the warning only fires on uses after the pragma. My thought was that could be a way to allow the header defining the macro to use it in some limited set of expansions. i.e:

// foo.h

//We only frobble on arm...
#if __is_target_arch(arm)
#define FROBBLE 1
#endif

#if FROBBLE
...
#endif
#pragma clang header_unsafe(FROBBLE, "Nobody else gets to Frobble in headers but me!")

Then in any user code, including foo.h causes FROBBLE to warn in all subsequent included sources.

The concrete use case for this is actually macOS Catalyst. Apple provides TargetConditionals.h which defines TARGET_OS_* macros for each target operating system.

The problem is, with Catalyst, you might be building a codebase that is going to run on macOS, but needs to be API & ABI compatible with iOS. So we want the ability to warn on those macros being used in headers, and instead encourage people to use API availability markup.

clang/lib/Lex/Preprocessor.cpp
1416–1435

+1

Will work on that.

clang/test/Lexer/Inputs/unsafe-macro-2.h
24–27

I kinda waffled on this myself. My thought was to treat this similarly to how we handle the macro redefinition warning. If you undef, you're kind of claiming the macro as your own and all bets are off...

That said, my next clang extension closes that loop hole too:
https://github.com/llvm-beanz/llvm-project/commit/f0a5216e18f5ee0883039095169bd380295b1de0

beanz updated this revision to Diff 365322.Aug 9 2021, 5:20 PM

Addressing feedback by @aaron.ballman.

If the pattern I used for the note diagnostic is good here, I'll apply it to the deprecated extension too.

beanz updated this revision to Diff 365324.Aug 9 2021, 5:24 PM

Updating documentation to be more clear about the behavior.

aaron.ballman added inline comments.Aug 10 2021, 4:54 AM
clang/test/Lexer/Inputs/unsafe-macro-2.h
24–27

So header_unsafe is "diagnose if someone expands this macro from outside the main source file" and final is "diagnose if someone defines or undefines this macro anywhere", correct? Would it make sense to have a shorthand to combine these effects for a "fully reserved" macro identifier (#pragma clang reserve_macro(IDENT[, msg]) as a strawman)?

aaron.ballman added inline comments.Aug 10 2021, 4:58 AM
clang/include/clang/Basic/DiagnosticLexKinds.td
539
beanz added inline comments.Aug 10 2021, 6:42 AM
clang/test/Lexer/Inputs/unsafe-macro-2.h
24–27

My thought process for implementing them separately was that final would be useful independent of header_unsafe. I could, for example, see applying final to macros like MIN and MAX, where they can be safely used anywhere, but you really don’t want multiple definitions floating around.

aaron.ballman added inline comments.Aug 10 2021, 7:29 AM
clang/test/Lexer/Inputs/unsafe-macro-2.h
24–27

FWIW, I agree that having separation is useful -- I think these serve orthogonal (but related) purposes: macros which can only be used by "user code", and macros which cannot be redefined or undefined. I was thinking that would be an additional pragma instead of a replacement for the two proposed.

I should probably tell you my use cases so we're both on the same page. One of the most frustrating problems with trying to write a highly portable library is the fact that I have to worry about users defining macros that may conflict with my identifiers (like function names, structure names, template names, etc), but I have no way to reserve those identifiers. I'm hopeful we can find a way that I can "protect" those identifiers in a library with an extension that basically says "you can't use these identifiers for macro purposes without breaking me". I think that's a combination of header_unsafe and final -- I don't want other libraries to start using macros with the names of my library's functions if my header has been included somewhere, and I don't want user code defining macros that may conflict with my library.

beanz added inline comments.Aug 10 2021, 7:32 AM
clang/test/Lexer/Inputs/unsafe-macro-2.h
24–27

Ah! That makes perfect sense. I think I misunderstood your comment. Totally agree, having a shorthand that groups both together would be super useful. I suspect we'll have some uses where we will want both final and header_unsafe too!

aaron.ballman added inline comments.Aug 10 2021, 7:57 AM
clang/docs/LanguageExtensions.rst
3941

May want to put something in here along the lines of: Redefining the macro or undefining the macro will not be diagnosed, nor will expansion of the macro within the main source file.

clang/include/clang/Basic/DiagnosticLexKinds.td
535

Should this get its own warning flag within the pedantic macros group? (Otherwise, it's awkward to try to silence this diagnostic while retaining some of the other ones like deprecated pragma.)

clang/test/Lexer/Inputs/unsafe-macro-2.h
24–27

Excellent! FWIW, I don't think the combined pragma has any impact on the behavior of this one.

beanz updated this revision to Diff 365487.Aug 10 2021, 8:28 AM

Updated docs, added header-unsafe-macro diag group, and added test case to verify that the -Wpedantic-macros warnings don't trip on eachother.

The technical bits all LGTM, thank you for the functionality! There was a comment on IRC that header_unsafe might be confusing to users as it may suggest that the *header* is unsafe rather than the use of the macro. I don't know if we resolved the naming question on IRC or whether that's something you'd like to bikeshed on more before we land this? I'll mark as approved once we're comfortable with the name.

Btw, I think you should add something to the release notes to alert folks to all of the awesome new preprocessor functionality.

beanz added a subscriber: lebedev.ri.

+@lebedev.ri

@aaron.ballman thank you for all the feedback and support!

I'm not really sure where to go on the naming. I'm not attached to header_unsafe, and totally understand the confusion. I don't really love the reserved* wording either as it doesn't really convey meaning.

For my other patch I like final because it means something similar to what the final keyword means in C++ and other languages (i.e. don't change this again).

For this I also thought about variations on restrict to signify restrictions on the macro's expansion contexts.

The goal is to warn on expansions that are outside the main source file, so maybe something like restrict_header_expansion?

Very open to feedback here on making clear naming. After all naming is one of the hardest problems in computer science :)

+@lebedev.ri

@aaron.ballman thank you for all the feedback and support!

I'm not really sure where to go on the naming. I'm not attached to header_unsafe, and totally understand the confusion. I don't really love the reserved* wording either as it doesn't really convey meaning.

We can spitball some ideas, and interested people can chime in with their opinions. I don't want to hold up the functionality for overly long on bikeshedding the name, so if at some point it starts to feel like we should pick one and go with it, we can do that. Sound reasonable?

For my other patch I like final because it means something similar to what the final keyword means in C++ and other languages (i.e. don't change this again).

For this I also thought about variations on restrict to signify restrictions on the macro's expansion contexts.

The goal is to warn on expansions that are outside the main source file, so maybe something like restrict_header_expansion?

Very open to feedback here on making clear naming. After all naming is one of the hardest problems in computer science :)

I think restrict_header_expansion is an improvement, but rather wordy. Maybe restrict_expansion instead? Paging @rsmith, who is often very good at naming questions.

beanz updated this revision to Diff 367851.Aug 20 2021, 12:00 PM

Renaming to restrict_expansion

beanz updated this revision to Diff 367856.Aug 20 2021, 12:03 PM

Fixing documentation line wrapping.

beanz updated this revision to Diff 367861.Aug 20 2021, 12:08 PM

Cleaning up documentation wording to make more sense with the rename

beanz updated this revision to Diff 367863.Aug 20 2021, 12:10 PM

Missed updating the flag name.

This revision is now accepted and ready to land.Aug 23 2021, 5:42 AM
beanz retitled this revision from Implement #pragma clang header_unsafe to Implement #pragma clang restrict_expansion.Aug 23 2021, 9:32 AM
beanz edited the summary of this revision. (Show Details)
beanz edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.