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

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D136011#3870677 , @labath wrote:

> In D136011#3866854 , @aeubanks 
> wrote:
>
>> In 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 , @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

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Arthur Eubanks via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGba8ded6820fa: [lldb] Dont check environment default 
char signedness when creating clang type… (authored by aeubanks).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136011

Files:
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/test/API/commands/expression/char/TestExprsChar.py
  lldb/test/API/commands/expression/char/main.cpp


Index: lldb/test/API/commands/expression/char/main.cpp
===
--- lldb/test/API/commands/expression/char/main.cpp
+++ lldb/test/API/commands/expression/char/main.cpp
@@ -1,5 +1,9 @@
 #include 
 
+char g = 0;
+signed char gs = 0;
+unsigned char gu = 0;
+
 int foo(char c) { return 1; }
 int foo(signed char c) { return 2; }
 int foo(unsigned char c) { return 3; }
Index: lldb/test/API/commands/expression/char/TestExprsChar.py
===
--- lldb/test/API/commands/expression/char/TestExprsChar.py
+++ lldb/test/API/commands/expression/char/TestExprsChar.py
@@ -14,30 +14,15 @@
 self.expect_expr("foo(c)", result_value="1")
 self.expect_expr("foo(sc)", result_value="2")
 self.expect_expr("foo(uc)", result_value="3")
+self.expect_expr("g", result_type="char")
+self.expect_expr("gs", result_type="signed char")
+self.expect_expr("gu", result_type="unsigned char")
 
 def test_default_char(self):
 self.do_test()
 
-@skipIf(oslist=["linux"], archs=["aarch64", "arm"], 
bugnumber="llvm.org/pr23069")
-@expectedFailureAll(
-archs=[
-"powerpc64le",
-"s390x"],
-bugnumber="llvm.org/pr23069")
 def test_signed_char(self):
 self.do_test(dictionary={'CFLAGS_EXTRAS': '-fsigned-char'})
 
-@expectedFailureAll(
-archs=[
-"i[3-6]86",
-"x86_64",
-"arm64",
-'arm64e',
-'armv7',
-'armv7k',
-'arm64_32'],
-bugnumber="llvm.org/pr23069, ")
-@expectedFailureAll(triple='mips*', bugnumber="llvm.org/pr23069")
-@expectedFailureAll(oslist=['windows'], archs=['aarch64'], 
bugnumber="llvm.org/pr23069")
 def test_unsigned_char(self):
 self.do_test(dictionary={'CFLAGS_EXTRAS': '-funsigned-char'})
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -1063,7 +1063,7 @@
 break;
 
   case DW_ATE_signed_char:
-if (ast.getLangOpts().CharIsSigned && type_name == "char") {
+if (type_name == "char") {
   if (QualTypeMatchesBitSize(bit_size, ast, ast.CharTy))
 return GetType(ast.CharTy);
 }
@@ -1115,7 +1115,7 @@
 break;
 
   case DW_ATE_unsigned_char:
-if (!ast.getLangOpts().CharIsSigned && type_name == "char") {
+if (type_name == "char") {
   if (QualTypeMatchesBitSize(bit_size, ast, ast.CharTy))
 return GetType(ast.CharTy);
 }


Index: lldb/test/API/commands/expression/char/main.cpp
===
--- lldb/test/API/commands/expression/char/main.cpp
+++ lldb/test/API/commands/expression/char/main.cpp
@@ -1,5 +1,9 @@
 #include 
 
+char g = 0;
+signed char gs = 0;
+unsigned char gu = 0;
+
 int foo(char c) { return 1; }
 int foo(signed char c) { return 2; }
 int foo(unsigned char c) { return 3; }
Index: lldb/test/API/commands/expression/char/TestExprsChar.py
===
--- lldb/test/API/commands/expression/char/TestExprsChar.py
+++ lldb/test/API/commands/expression/char/TestExprsChar.py
@@ -14,30 +14,15 @@
 self.expect_expr("foo(c)", result_value="1")
 self.expect_expr("foo(sc)", result_value="2")
 self.expect_expr("foo(uc)", result_value="3")
+self.expect_expr("g", result_type="char")
+self.expect_expr("gs", result_type="signed char")
+self.expect_expr("gu", result_type="unsigned char")
 
 def test_default_char(self):
 self.do_test()
 
-@skipIf(oslist=["linux"], archs=["aarch64", "arm"], bugnumber="llvm.org/pr23069")
-@expectedFailureAll(
-archs=[
-"powerpc64le",
-"s390x"],
-bugnumber="llvm.org/pr23069")
 def test_signed_char(self):
 self.do_test(dictionary={'CFLAGS_EXTRAS': '-fsigned-char'})
 
-@expectedFailureAll(
-archs=[
-"i[3-6]86",
-"x86_64",
-"arm64",
-'arm64e',
-'armv7',
-'armv7k',
-'arm64_32'],
-bugnumber="llvm.org/pr23069, ")
-@expectedFailureAll(triple='mips*', bugnumber="llvm.org/pr23069")
-  

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

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

In D136011#3866854 , @aeubanks wrote:

> In 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 , @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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136011

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D136011#3862150 , @labath wrote:

> In D136011#3860637 , @dblaikie 
> wrote:
>
>> I think the place where this will go wrong is in terms of how lldb renders 
>> `char` values on non-default-char-signedness programs (it'll render them as 
>> the default-char-signedness, which might be confusing to a user - since 
>> they'll be looking at literals, etc, that are the other signedness) and how 
>> lldb will interpret char literals (though that'll already be wrong - since 
>> the literals are already being parsed with the default-char-signedness, I 
>> think).
>
> Yes, I'm pretty sure that will happen. OTOH, I don't think there's any value 
> to fix this in a completely satisfactory way. Like, if the whole program was 
> consistently with the non-default signedness, we could try to detect it and 
> then configure the internal AST defaults accordingly. But that's hard to 
> detect, and I'd be surprised if most programs are completely homogeneous like 
> this.
>
> So, overall, I quite like this fix.

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.

Works for me. (though lldb's sufficiently out of my wheelhouse I'll leave it to 
others to approve)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136011

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Arthur Eubanks via Phabricator via lldb-commits]

aeubanks added a comment.

In D136011#3862150 , @labath wrote:

> In D136011#3860637 , @dblaikie 
> wrote:
>
>> I think the place where this will go wrong is in terms of how lldb renders 
>> `char` values on non-default-char-signedness programs (it'll render them as 
>> the default-char-signedness, which might be confusing to a user - since 
>> they'll be looking at literals, etc, that are the other signedness) and how 
>> lldb will interpret char literals (though that'll already be wrong - since 
>> the literals are already being parsed with the default-char-signedness, I 
>> think).
>
> Yes, I'm pretty sure that will happen. OTOH, I don't think there's any value 
> to fix this in a completely satisfactory way. Like, if the whole program was 
> consistently with the non-default signedness, we could try to detect it and 
> then configure the internal AST defaults accordingly. But that's hard to 
> detect, and I'd be surprised if most programs are completely homogeneous like 
> this.
>
> So, overall, I quite like this fix.

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.

>> I'm curious why there were all those expected failures re: PR23069. Were 
>> they not using the default char signedness? Or is the test using explicit 
>> signedness, and so whatever platforms happen not to have the explicit value 
>> sa their default are/were failing?
>
> Yes, I'm pretty sure that's the case.
>
> In D136011#3861792 , @DavidSpickett 
> wrote:
>
>>> With -f(un)signed-char, the die corresponding to "char" may be the wrong 
>>> DW_ATE_(un)signed_char.
>>
>> As the producer of the DWARF (so, clang for example) is this correct by the 
>> existing rules?
>
> Yes, because DWARF never expected people will be round-tripping the debug 
> info back into C(++). As far as it is concerned, it has correctly given you 
> the signedness of the type.

"wrong" is inaccurate, I've updated the summary


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136011

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D136011#3860637 , @dblaikie wrote:

> I think the place where this will go wrong is in terms of how lldb renders 
> `char` values on non-default-char-signedness programs (it'll render them as 
> the default-char-signedness, which might be confusing to a user - since 
> they'll be looking at literals, etc, that are the other signedness) and how 
> lldb will interpret char literals (though that'll already be wrong - since 
> the literals are already being parsed with the default-char-signedness, I 
> think).

Yes, I'm pretty sure that will happen. OTOH, I don't think there's any value to 
fix this in a completely satisfactory way. Like, if the whole program was 
consistently with the non-default signedness, we could try to detect it and 
then configure the internal AST defaults accordingly. But that's hard to 
detect, and I'd be surprised if most programs are completely homogeneous like 
this.

So, overall, I quite like this fix.

> I'm curious why there were all those expected failures re: PR23069. Were they 
> not using the default char signedness? Or is the test using explicit 
> signedness, and so whatever platforms happen not to have the explicit value 
> sa their default are/were failing?

Yes, I'm pretty sure that's the case.

In D136011#3861792 , @DavidSpickett 
wrote:

>> With -f(un)signed-char, the die corresponding to "char" may be the wrong 
>> DW_ATE_(un)signed_char.
>
> As the producer of the DWARF (so, clang for example) is this correct by the 
> existing rules?

Yes, because DWARF never expected people will be round-tripping the debug info 
back into C(++). As far as it is concerned, it has correctly given you the 
signedness of the type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136011

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

> With -f(un)signed-char, the die corresponding to "char" may be the wrong 
> DW_ATE_(un)signed_char.

As the producer of the DWARF (so, clang for example) is this correct by the 
existing rules?

As I understand it so far, if the compiler is using "char" (no sign chosen) it 
can use either `DW_ATE_unsigned_char` or `DW_ATE_signed_char`. So lldb cannot 
trust the choice the compiler made to tell it what plain `char` signedness 
should be?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136011

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Arthur Eubanks via Phabricator via lldb-commits]

aeubanks updated this revision to Diff 468118.
aeubanks added a comment.

update test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136011

Files:
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/test/API/commands/expression/char/TestExprsChar.py
  lldb/test/API/commands/expression/char/main.cpp


Index: lldb/test/API/commands/expression/char/main.cpp
===
--- lldb/test/API/commands/expression/char/main.cpp
+++ lldb/test/API/commands/expression/char/main.cpp
@@ -1,5 +1,9 @@
 #include 
 
+char g = 0;
+signed char gs = 0;
+unsigned char gu = 0;
+
 int foo(char c) { return 1; }
 int foo(signed char c) { return 2; }
 int foo(unsigned char c) { return 3; }
Index: lldb/test/API/commands/expression/char/TestExprsChar.py
===
--- lldb/test/API/commands/expression/char/TestExprsChar.py
+++ lldb/test/API/commands/expression/char/TestExprsChar.py
@@ -14,30 +14,15 @@
 self.expect_expr("foo(c)", result_value="1")
 self.expect_expr("foo(sc)", result_value="2")
 self.expect_expr("foo(uc)", result_value="3")
+self.expect_expr("g", result_type="char")
+self.expect_expr("gs", result_type="signed char")
+self.expect_expr("gu", result_type="unsigned char")
 
 def test_default_char(self):
 self.do_test()
 
-@skipIf(oslist=["linux"], archs=["aarch64", "arm"], 
bugnumber="llvm.org/pr23069")
-@expectedFailureAll(
-archs=[
-"powerpc64le",
-"s390x"],
-bugnumber="llvm.org/pr23069")
 def test_signed_char(self):
 self.do_test(dictionary={'CFLAGS_EXTRAS': '-fsigned-char'})
 
-@expectedFailureAll(
-archs=[
-"i[3-6]86",
-"x86_64",
-"arm64",
-'arm64e',
-'armv7',
-'armv7k',
-'arm64_32'],
-bugnumber="llvm.org/pr23069, ")
-@expectedFailureAll(triple='mips*', bugnumber="llvm.org/pr23069")
-@expectedFailureAll(oslist=['windows'], archs=['aarch64'], 
bugnumber="llvm.org/pr23069")
 def test_unsigned_char(self):
 self.do_test(dictionary={'CFLAGS_EXTRAS': '-funsigned-char'})
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -1063,7 +1063,7 @@
 break;
 
   case DW_ATE_signed_char:
-if (ast.getLangOpts().CharIsSigned && type_name == "char") {
+if (type_name == "char") {
   if (QualTypeMatchesBitSize(bit_size, ast, ast.CharTy))
 return GetType(ast.CharTy);
 }
@@ -1115,7 +1115,7 @@
 break;
 
   case DW_ATE_unsigned_char:
-if (!ast.getLangOpts().CharIsSigned && type_name == "char") {
+if (type_name == "char") {
   if (QualTypeMatchesBitSize(bit_size, ast, ast.CharTy))
 return GetType(ast.CharTy);
 }


Index: lldb/test/API/commands/expression/char/main.cpp
===
--- lldb/test/API/commands/expression/char/main.cpp
+++ lldb/test/API/commands/expression/char/main.cpp
@@ -1,5 +1,9 @@
 #include 
 
+char g = 0;
+signed char gs = 0;
+unsigned char gu = 0;
+
 int foo(char c) { return 1; }
 int foo(signed char c) { return 2; }
 int foo(unsigned char c) { return 3; }
Index: lldb/test/API/commands/expression/char/TestExprsChar.py
===
--- lldb/test/API/commands/expression/char/TestExprsChar.py
+++ lldb/test/API/commands/expression/char/TestExprsChar.py
@@ -14,30 +14,15 @@
 self.expect_expr("foo(c)", result_value="1")
 self.expect_expr("foo(sc)", result_value="2")
 self.expect_expr("foo(uc)", result_value="3")
+self.expect_expr("g", result_type="char")
+self.expect_expr("gs", result_type="signed char")
+self.expect_expr("gu", result_type="unsigned char")
 
 def test_default_char(self):
 self.do_test()
 
-@skipIf(oslist=["linux"], archs=["aarch64", "arm"], bugnumber="llvm.org/pr23069")
-@expectedFailureAll(
-archs=[
-"powerpc64le",
-"s390x"],
-bugnumber="llvm.org/pr23069")
 def test_signed_char(self):
 self.do_test(dictionary={'CFLAGS_EXTRAS': '-fsigned-char'})
 
-@expectedFailureAll(
-archs=[
-"i[3-6]86",
-"x86_64",
-"arm64",
-'arm64e',
-'armv7',
-'armv7k',
-'arm64_32'],
-bugnumber="llvm.org/pr23069, ")
-@expectedFailureAll(triple='mips*', bugnumber="llvm.org/pr23069")
-@expectedFailureAll(oslist=['windows'], archs=['aarch64'], bugnumber="llvm.org/pr23069")
 def 

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

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

I think the place where this will go wrong is in terms of how lldb renders 
`char` values on non-default-char-signedness programs (it'll render them as the 
default-char-signedness, which might be confusing to a user - since they'll be 
looking at literals, etc, that are the other signedness) and how lldb will 
interpret char literals (though that'll already be wrong - since the literals 
are already being parsed with the default-char-signedness, I think).

I'm curious why there were all those expected failures re: PR23069. Were they 
not using the default char signedness? Or is the test using explicit 
signedness, and so whatever platforms happen not to have the explicit value sa 
their default are/were failing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136011

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Arthur Eubanks via Phabricator via lldb-commits]

aeubanks added a comment.

not 100% sure this is the right fix


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136011

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Arthur Eubanks via Phabricator via lldb-commits]

aeubanks created this revision.
Herald added a project: All.
aeubanks requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

With -f(un)signed-char, the die corresponding to "char" may be the wrong 
DW_ATE_(un)signed_char. Ultimately we can determine whether a type is the 
unspecified signedness char by looking if its name is "char" (as opposed to 
"signed char"/"unsigned char").

Fixes #23443


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D136011

Files:
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/test/API/commands/expression/char/TestExprsChar.py
  lldb/test/API/commands/expression/char/main.cpp


Index: lldb/test/API/commands/expression/char/main.cpp
===
--- lldb/test/API/commands/expression/char/main.cpp
+++ lldb/test/API/commands/expression/char/main.cpp
@@ -1,5 +1,7 @@
 #include 
 
+char g = 0;
+
 int foo(char c) { return 1; }
 int foo(signed char c) { return 2; }
 int foo(unsigned char c) { return 3; }
Index: lldb/test/API/commands/expression/char/TestExprsChar.py
===
--- lldb/test/API/commands/expression/char/TestExprsChar.py
+++ lldb/test/API/commands/expression/char/TestExprsChar.py
@@ -14,30 +14,13 @@
 self.expect_expr("foo(c)", result_value="1")
 self.expect_expr("foo(sc)", result_value="2")
 self.expect_expr("foo(uc)", result_value="3")
+self.expect_expr("g", result_type="char")
 
 def test_default_char(self):
 self.do_test()
 
-@skipIf(oslist=["linux"], archs=["aarch64", "arm"], 
bugnumber="llvm.org/pr23069")
-@expectedFailureAll(
-archs=[
-"powerpc64le",
-"s390x"],
-bugnumber="llvm.org/pr23069")
 def test_signed_char(self):
 self.do_test(dictionary={'CFLAGS_EXTRAS': '-fsigned-char'})
 
-@expectedFailureAll(
-archs=[
-"i[3-6]86",
-"x86_64",
-"arm64",
-'arm64e',
-'armv7',
-'armv7k',
-'arm64_32'],
-bugnumber="llvm.org/pr23069, ")
-@expectedFailureAll(triple='mips*', bugnumber="llvm.org/pr23069")
-@expectedFailureAll(oslist=['windows'], archs=['aarch64'], 
bugnumber="llvm.org/pr23069")
 def test_unsigned_char(self):
 self.do_test(dictionary={'CFLAGS_EXTRAS': '-funsigned-char'})
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -1063,7 +1063,7 @@
 break;
 
   case DW_ATE_signed_char:
-if (ast.getLangOpts().CharIsSigned && type_name == "char") {
+if (type_name == "char") {
   if (QualTypeMatchesBitSize(bit_size, ast, ast.CharTy))
 return GetType(ast.CharTy);
 }
@@ -1115,7 +1115,7 @@
 break;
 
   case DW_ATE_unsigned_char:
-if (!ast.getLangOpts().CharIsSigned && type_name == "char") {
+if (type_name == "char") {
   if (QualTypeMatchesBitSize(bit_size, ast, ast.CharTy))
 return GetType(ast.CharTy);
 }


Index: lldb/test/API/commands/expression/char/main.cpp
===
--- lldb/test/API/commands/expression/char/main.cpp
+++ lldb/test/API/commands/expression/char/main.cpp
@@ -1,5 +1,7 @@
 #include 
 
+char g = 0;
+
 int foo(char c) { return 1; }
 int foo(signed char c) { return 2; }
 int foo(unsigned char c) { return 3; }
Index: lldb/test/API/commands/expression/char/TestExprsChar.py
===
--- lldb/test/API/commands/expression/char/TestExprsChar.py
+++ lldb/test/API/commands/expression/char/TestExprsChar.py
@@ -14,30 +14,13 @@
 self.expect_expr("foo(c)", result_value="1")
 self.expect_expr("foo(sc)", result_value="2")
 self.expect_expr("foo(uc)", result_value="3")
+self.expect_expr("g", result_type="char")
 
 def test_default_char(self):
 self.do_test()
 
-@skipIf(oslist=["linux"], archs=["aarch64", "arm"], bugnumber="llvm.org/pr23069")
-@expectedFailureAll(
-archs=[
-"powerpc64le",
-"s390x"],
-bugnumber="llvm.org/pr23069")
 def test_signed_char(self):
 self.do_test(dictionary={'CFLAGS_EXTRAS': '-fsigned-char'})
 
-@expectedFailureAll(
-archs=[
-"i[3-6]86",
-"x86_64",
-"arm64",
-'arm64e',
-'armv7',
-'armv7k',
-'arm64_32'],
-bugnumber="llvm.org/pr23069, ")
-@expectedFailureAll(triple='mips*', bugnumber="llvm.org/pr23069")
-@expectedFailureAll(oslist=['windows'], archs=['aarch64'], bugnumber="llvm.org/pr23069")
 def test_unsigned_char(self):