[PATCH] D144323: [libunwind][Modules] Add unwind_arm_ehabi.h and unwind_itanium.h to the unwind module)

[Ian Anderson via Phabricator via llvm-commits]

iana added inline comments.


================
Comment at: libunwind/include/unwind_arm_ehabi.h:16-18
+#include <__libunwind_config.h>
+
+#ifdef _LIBUNWIND_ARM_EHABI
----------------
ldionne wrote:
> iana wrote:
> > ldionne wrote:
> > > Why is this needed? We only include those after checking `#if defined(_LIBUNWIND_ARM_EHABI)`.
> > The module will build all of its headers, so without these guards you'll get a lot of redeclaration errors between the two unwind_ headers.
> > 
> > It might maybe work to put them in independent modules and mark them as conflicting. I don't really think that's the right thing to do since these two headers are just implementation details for unwind.h.
> > 
> > We could make them textual, but then they don't get put in the pcm which isn't what we want.
> > 
> > We could make them excluded and then one or the other should be pulled into unwind when it builds, but that's a fragile mechanism since some other module could unexpectedly include them. It's also not really what we're trying to do, they should be part of unwind and not really excluded.
> > 
> > So I think the right thing to do is make each of these headers standalone and able to be built in the same module.
> The problem is that this is a general pattern we have in a lot of places. Today, we use headers as a mechanism to (quite textually) separate code into a different physical location. These headers are implementation details and they are never intended to be included directly by the user. I'm not a modules expert, but it looks to me like those are effectively textual headers that are meant to be copy-pasted into `unwind.h` depending on the API we want to provide. If so, we should make them textual even if that has a negative impact on the generated PCM, since that is the proper semantics of those headers.
Err, it would be in the unwind PCM since unwind.h includes it, it just has the potential to get copied into other PCMs. @Bigcheese suggested we make it `private textual` to prevent that, let me give it a try. I still feel a little icky about making the module behavior differ from textual behavior (modules will emit an error if the unwind_ headers are directly included but textual won't), but it's not so bad.

I guess it's open to interpretation, but I see textual headers as things like assert.h where it's expected that you can include it multiple times with different values of `NDEBUG` in your code, or for X Macro generator headers. The unwind_ headers don't really fit either of those descriptions which is why I feel like they're a bad fit for textual, especially when it's a relatively small change to make them standalone.

That said, I can be out voted, what's really important to me is these belong to a module. I'll give `private textual` a try and see if it works out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144323