[PATCH] D107095: Implement #pragma clang header_unsafe

[Chris Bieneman via Phabricator via cfe-commits]

beanz marked 5 inline comments as done.
beanz added a comment.

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



================
Comment at: clang/docs/LanguageExtensions.rst:3913
+Clang supports the pragma ``#pragma clang header_unsafe``, which can be used to
+mark macros as unsafe to use in headers. This can be valuable when providing
+headers with ABI stability requirements. For example:
----------------
aaron.ballman wrote:
> 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`?
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.


================
Comment at: clang/lib/Lex/Preprocessor.cpp:1416-1434
+void Preprocessor::emitMacroDeprecationWarning(const Token &Identifier) {
+  auto DepMsg = getMacroDeprecationMsg(Identifier.getIdentifierInfo());
+  if (!DepMsg)
+    Diag(Identifier, diag::warn_pragma_deprecated_macro_use)
+        << Identifier.getIdentifierInfo() << 0;
+  else
+    Diag(Identifier, diag::warn_pragma_deprecated_macro_use)
----------------
aaron.ballman wrote:
> 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?
+1

Will work on that.


================
Comment at: clang/test/Lexer/Inputs/unsafe-macro-2.h:23-26
+// not-expected-warning at +1{{macro 'UNSAFE_MACRO_2' has been marked as unsafe for use in headers}}
+#undef UNSAFE_MACRO_2
+// not-expected-warning at +1{{macro 'UNSAFE_MACRO_2' has been marked as unsafe for use in headers}}
+#define UNSAFE_MACRO_2 2
----------------
aaron.ballman wrote:
> 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.
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107095