[Lldb-commits] [PATCH] D136011: [lldb] Don't check environment default char signedness when creating clang type for "char"

Mon Oct 24 12:17:23 PDT 2022

dblaikie added a comment.

In D136011#3870677 <https://reviews.llvm.org/D136011#3870677>, @labath wrote:

> In D136011#3866854 <https://reviews.llvm.org/D136011#3866854>, @aeubanks wrote:
>
>> In D136011#3862150 <https://reviews.llvm.org/D136011#3862150>, @labath wrote:
>>
>>> 
>>
>> Maybe looking for a "char" entry and setting `ast.getLangOpts().CharIsSigned` from it would probably work in most cases, but not sure that it's worth the effort. Rendering the wrong signedness for chars doesn't seem like the worst thing, and this patch improves things (as the removed `expectedFailureAll` show, plus this helps with some testing of lldb I've been doing). I can add a TODO if you'd like.
>
> It's not just a question of rendering. This can result in wrong/unexpected results of expressions as well. Something as simple as `p (int)unspecified_char_value` can return a different result depending on whether `unspecified_char_value` is considered signed or unsigned.
> However, I am not convinced that we would be better off trying to detect this would, as that detection can fail as well, only in more unpredictable ways.
>
> In D136011#3868755 <https://reviews.llvm.org/D136011#3868755>, @dblaikie wrote:
>
>> Yeah, this line of reasoning (non-homogenaeity) is one I'm on board with, thanks for the framing. Basically I/we can think of the debugger's expression context as some code that's compiled with the default char signedness always. Since char signedness isn't part of the ABI, bits of the program can be built with one and bits with the other - and the debugger is just a bit that's built with the default.
>
> Are you sure about the ABI part? I guess it may depend on how you define the ABI.. It definitely does not affect the calling conventions, but I'm pretty sure it would be an ODR violation if you even had a simple function like `int to_int(char c) { return c; }` in a header, and compiled it with both -fsigned and -funsigned-char. Not that this will stop people from doing it, but the most likely scenario for this is that your linking your application with the C library compiled in a different mode, where it is kinda ok, because the C library does not have many inline functions, and doesn't do much char manipulation.

"ODR violation" gets a bit fuzzy - but yeah, you'd get a different int out of that function. My point was since apps would already end up with a mix of signed and unsigned most likely, the debugger expression evaluation is one of those things mixed one way rather than the other.

But yeah, it's all a bit awkward (& probably will be more for googlers where everything's built with the non-default signedness - and now the expression evaluator will do the other thing/not that - but hopefully a rather small number of users ever notice or care about this). I guess char buffers for raw/binary data will be a bit more annoying to look at, dumped as signed integers instead of unsigned...

Anyway, yeah, it's all a bit awkward but this seems like the best thing for now. Thanks for weighing in!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136011