This is an archive of the discontinued LLVM Phabricator instance.

[libunwind][include] Add some missing definitions to <unwind.h>.
AbandonedPublic

Authored by cdavis5x on Aug 7 2018, 3:49 PM.

Details

Summary

Add these declarations which should be present in <unwind.h>,
but aren't. Not that it matters, since most of the time we'll be using
Clang's <unwind.h> anyway.

Event Timeline

cdavis5x created this revision.Aug 7 2018, 3:49 PM

NetBSD uses typedef void *_Unwind_Ptr; unconditionally in its <unwind.h>... if that has to be matched.

Additionally: typedef long _Unwind_Word;.

mstorsjo added inline comments.Aug 7 2018, 11:17 PM
include/unwind.h
50

What other reference is this list of typedefs for _Unwind_Ptr based on? I don't see any of these cases in clang's unwind.h at least.

185

Minor nitpick: Would it make sense to move the typedef _Unwind_Personality_Fn __personality_routine; out of the ifdef, since the same one can be used for both cases?

cdavis5x added inline comments.Aug 8 2018, 8:34 AM
include/unwind.h
50

Where did I get those from...? These seem to be for ARM EHABI, but I can't find them anymore in the GCC source. They aren't in Clang's header, either. I wrote this a while ago... did something change in the meantime?

I could probably get away with removing those special cases if we don't really need them.

cdavis5x updated this revision to Diff 159731.Aug 8 2018, 8:48 AM
  • Add NetBSD-specific definitions.
  • Pull out common declaration of __personality_routine.
cdavis5x marked an inline comment as done.Aug 8 2018, 8:49 AM

NetBSD uses typedef void *_Unwind_Ptr; unconditionally in its <unwind.h>... if that has to be matched.

Done.

Additionally: typedef long _Unwind_Word;.

Done.

NetBSD uses typedef void *_Unwind_Ptr; unconditionally in its <unwind.h>... if that has to be matched.

Done.

Additionally: typedef long _Unwind_Word;.

Done.

I recommend to get a review of this by @joerg

@cdavis5x I presume the fact that this one turned out tricky is blocking submitting the SEH unwinding patch.

Would it be worth to rework that patch to just use the basic types just like libunwind does today, e.g. having _Unwind_GetRegionStart return plain uintptr_t instead of _Unwind_Ptr, which also would be consistent with the existing entry points in Unwind-EHABI.cpp, Unwind-sjlj.c and UnwindLevel1.c, in order to be able to submit it before this one gets sorted out?

@cdavis5x I presume the fact that this one turned out tricky is blocking submitting the SEH unwinding patch.

Would it be worth to rework that patch to just use the basic types just like libunwind does today, e.g. having _Unwind_GetRegionStart return plain uintptr_t instead of _Unwind_Ptr, which also would be consistent with the existing entry points in Unwind-EHABI.cpp, Unwind-sjlj.c and UnwindLevel1.c, in order to be able to submit it before this one gets sorted out?

Sounds reasonable.

Ping.

Are y'all waiting for me to do something?

Is there a reason for defining them? As in: does anything outside libunwind use them? I haven't seen such software yet.

This doesn't seem like a great idea to me, it's a negligible amount of type safety gained over using stdint.h types most people are more familiar with anyway. If you need something niche like those typedefs it's probably better to include the header and define them yourself in your application.

From my point of view, the number of people who have idea about these types and why do they differ between ABIs and OSes is so low, that it's even hard to get someone to review it here.

rnk added a comment.Aug 30 2018, 2:57 PM

Do we still need or want this? It doesn't look like it would hurt.

kristina added a comment.EditedAug 30 2018, 4:17 PM

Probably not (to disambiguate, as in, it wouldn't hurt).

I'm personally against adding more and more typedefs for things that are just stdint/inttypes.h stuff and I say here the change is insignificant enough to warrant having to define even more types. It's not causing any problems but unless there are actual custom composite types that belong in that header, I don't see the benefit of them, if everyone starts wanting to typedef trivial types in unwind.h, it could be a maintainability issue. Not talking about this patch specifically but about a precedent of whether trivial types should get more and more aliases, polluting the header, and whether sticking to standard types and aliasing them in implementation is perhaps a better approach. Since the header gets installed everywhere and there isn't something like unifdef removing otherwise dead code. SEH patch was of much bigger significance so in that case it was fine (though I still like insisting on avoiding custom typedefs like DWORD for uint32_t just because rest of LLVM follows the same practice, while new composite types can be a necessity, more and more trivial type aliases isn't).

That's my stance on it. Patch itself is sound however, ignoring what I said above.

rnk added a comment.Aug 30 2018, 5:26 PM

So, basically, if we have no internal users for these typedefs, there's no reason to clutter up headers that get installed into system header directories everywhere with dead ifdefed out code. Sounds good to me, let's not do it.

cdavis5x abandoned this revision.Aug 30 2018, 6:07 PM

Consensus seems clear to me. Patch abandoned.