mstorsjo added a comment. In https://reviews.llvm.org/D39365#910590, @theraven wrote:
> 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 (https://reviews.llvm.org/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` (https://reviews.llvm.org/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 (https://reviews.llvm.org/D39251) - Make `unw_word_t` be `uin64_t` consistently everywhere (https://reviews.llvm.org/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. https://reviews.llvm.org/D39365 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
