This is an archive of the discontinued LLVM Phabricator instance.

[libunwind] Change unw_word_t to always have the same size as the pointer size
ClosedPublic

Authored by mstorsjo on Oct 27 2017, 2:13 AM.

Details

Summary

This matches the original libunwind API. This also unifies the type between ARM EHABI and the other configurations, and allows getting rid of a number of casts in log messages.

The cursor size updates for ppc and or1k are untested, but unw_proc_info_t shrinks by 4 uint64_t units on 32 bit platforms.

Diff Detail

Repository
rL LLVM

Event Timeline

mstorsjo created this revision.Oct 27 2017, 2:13 AM
rnk edited edge metadata.Oct 27 2017, 1:03 PM

Doesn't this change the ABI considerably? I suspect Apple cares about that.

Is remotely unwinding a 64-bit thread from a 32-bit process a concern? That's the main use case that forcing 64-bit words seems to enable.

In D39365#909706, @rnk wrote:

Doesn't this change the ABI considerably? I suspect Apple cares about that.

It does change the ABI of the lower level unw_* API yes, but it shouldn't be visible outside via the higher level APIs.

Is remotely unwinding a 64-bit thread from a 32-bit process a concern? That's the main use case that forcing 64-bit words seems to enable.

If that's a concern, then the other patch I propsed earlier would make things even more consistent, by using 64 bit words even on ARM EHABI: https://reviews.llvm.org/D39280

compnerd edited edge metadata.Oct 27 2017, 3:57 PM

@rnk given that the remote unwinder is completely unimplemented ATM, I think that this isn't a big concern. I'm not sure that this makes anything worse, but I do feel that it is making things better (especially since RemoteAddressSpace is currently mostly just a '// FIXME: implement').

This makes things worse for us. On CHERI, [u]intptr_t is a (typedef for a) built-in type that can hold a capability. Having unw_word_t be uintptr_t made LLVM's libunwind considerably easier to port than the HP unwinder would have been, because uintptr_t is a type that is able to hold either any integer-register type or a pointer, whereas neither uint32_t nor uint64_t is. This will be true for any architecture that supports any kind of fat-pointer representation.

What is the motivation for this change? It appears to be changing working code in such a way that removes future proofing.

Replacing integer casts with void* casts and using PRIxPTR consistently would make life easier, though the use of PRIxPTR vs PRIXPTR seems inconsistent (as is the original use of %x vs %X.

This makes things worse for us. On CHERI, [u]intptr_t is a (typedef for a) built-in type that can hold a capability. Having unw_word_t be uintptr_t

For understanding, I guess you meant "Having unw_word_t be uint64_t" here? Because othewise, that's exactly the change I'm doing - currently it's uint64_t while I'm proposing making it uintptr_t - that you're saying is easier to work with?

made LLVM's libunwind considerably easier to port than the HP unwinder would have been, because uintptr_t is a type that is able to hold either any integer-register type or a pointer, whereas neither uint32_t nor uint64_t is. This will be true for any architecture that supports any kind of fat-pointer representation.

What is the motivation for this change? It appears to be changing working code in such a way that removes future proofing.

Replacing integer casts with void* casts and using PRIxPTR consistently would make life easier,

The original root cause that triggered me to write this, is that currently unw_word_t is uint32_t for ARM EHABI but uint64_t for everything else. Since I want to add support for ARM/DWARF (D39251), this required adding casts in UnwindLevel1.c which currently implicitly assumes that unw_word_t is uint64_t. Instead of adding such casts, I made one patch to change it to consistently be uint64_t (D39280) even on ARM EHABI, but that got the review comment that unw_word_t should match uintptr_t, thus I wrote this patch.

So the options currently seem to be:

  • Keep the inconsistency and add ifdefs for the context size for ARM (which would be different for EHABI vs DWARF), where we already have ifdefs for the context size depending on if WMMX is enabled or not - that would make 4 different hardcoded sizes in __libunwind_config.h.
  • Make unw_word_t be uint32_t on ARM, uint64_t everywhere else (minimal change from status quo), add casts of one form or another in UnwindLevel1.c (D39251)
  • Make unw_word_t be uin64_t consistently everywhere (D39280)
  • Make unw_word_t be uintptr_t (this one)

I don't have too much of a preference, as long as people settle on one - so far every review has pointed me in a different direction.

though the use of PRIxPTR vs PRIXPTR seems inconsistent (as is the original use of %x vs %X.

Yes, I've kept these as inconsistent as they were originally - if peferred I can make the ones I touch consistently either upper or lower case.

This makes things worse for us. On CHERI, [u]intptr_t is a (typedef for a) built-in type that can hold a capability. Having unw_word_t be uintptr_t

For understanding, I guess you meant "Having unw_word_t be uint64_t" here? Because othewise, that's exactly the change I'm doing - currently it's uint64_t while I'm proposing making it uintptr_t - that you're saying is easier to work with?

Sorry - it looks as if I read the diff back to front. I seem to be less awake than I thought today...

Reading the diff the correct way around, this seems like a definite improvement.

though the use of PRIxPTR vs PRIXPTR seems inconsistent (as is the original use of %x vs %X.

Yes, I've kept these as inconsistent as they were originally - if peferred I can make the ones I touch consistently either upper or lower case.

I'd generally prefer PRIxPTR, because most of the time I either don't care or want to copy and paste for comparison with objdump output (which uses lower case).

Sorry - it looks as if I read the diff back to front. I seem to be less awake than I thought today...

Reading the diff the correct way around, this seems like a definite improvement.

Ok, thanks! Then it's just left to see if @rnk agrees with @compnerd 's reasoning.

though the use of PRIxPTR vs PRIXPTR seems inconsistent (as is the original use of %x vs %X.

Yes, I've kept these as inconsistent as they were originally - if peferred I can make the ones I touch consistently either upper or lower case.

I'd generally prefer PRIxPTR, because most of the time I either don't care or want to copy and paste for comparison with objdump output (which uses lower case).

Ok, I'll make it consistent in the next update or when committing.

rnk accepted this revision.Oct 30 2017, 9:49 AM

@rnk given that the remote unwinder is completely unimplemented ATM, I think that this isn't a big concern. I'm not sure that this makes anything worse, but I do feel that it is making things better (especially since RemoteAddressSpace is currently mostly just a '// FIXME: implement').

Sounds good to me, just wanted to check. :)

This revision is now accepted and ready to land.Oct 30 2017, 9:49 AM
This revision was automatically updated to reflect the committed changes.