[Lldb-commits] [PATCH] D134585: [lldb][COFF] Map symbols without base+complex type as 'Data' type

[Martin Storsjö via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8a67a05e9334: [lldb][COFF] Map symbols without base+complex 
type as Data type (authored by alvinhochun, committed by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134585

Files:
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
  lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml


Index: lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
@@ -15,7 +15,7 @@
 # CHECK-NEXT: 4294967295 Code   0x000180001020
0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasFunc
 # CHECK-NEXT: 4294967295   X Additional 0x000180003000
0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
 # CHECK-NEXT: 4294967295 Data   0x000180003004
0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasInt
-# CHECK-NEXT: 4294967295 Invalid0x000180003008
0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} internalInt
+# CHECK-NEXT: 4294967295 Data   0x000180003008
0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} internalInt
 # CHECK-EMPTY:
 
 # Test file generated with:
Index: lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
@@ -6,7 +6,7 @@
 
 # CHECK: Type File Address/Value {{.*}} SizeFlags   
Name
 # CHECK: Code 0x400010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} 
entry
-# CHECK:  0x400020000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} 
variable
+# CHECK: Data 0x400020000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} 
variable
 # CHECK: Absolute 0xdeadbeef0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} 
absolute_symbol
 
 --- !COFF
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -379,6 +379,13 @@
   if (complex_type == llvm::COFF::IMAGE_SYM_DTYPE_FUNCTION) {
 return lldb::eSymbolTypeCode;
   }
+  const auto base_type = coff_symbol_type & 0xff;
+  if (base_type == llvm::COFF::IMAGE_SYM_TYPE_NULL &&
+  complex_type == llvm::COFF::IMAGE_SYM_DTYPE_NULL) {
+// Unknown type. LLD and GNU ld uses this for variables on MinGW, so
+// consider these symbols to be data to enable printing.
+return lldb::eSymbolTypeData;
+  }
   return lldb::eSymbolTypeInvalid;
 }
 


Index: lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
@@ -15,7 +15,7 @@
 # CHECK-NEXT: 4294967295 Code   0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasFunc
 # CHECK-NEXT: 4294967295   X Additional 0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
 # CHECK-NEXT: 4294967295 Data   0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasInt
-# CHECK-NEXT: 4294967295 Invalid0x0001800030080x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} internalInt
+# CHECK-NEXT: 4294967295 Data   0x0001800030080x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} internalInt
 # CHECK-EMPTY:
 
 # Test file generated with:
Index: lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
@@ -6,7 +6,7 @@
 
 # CHECK: Type File Address/Value {{.*}} SizeFlags   Name
 # CHECK: Code 0x400010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
-# CHECK:  0x400020000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} variable
+# CHECK: Data 0x400020000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} variable
 # CHECK: Absolute 0xdeadbeef0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} absolute_symbol
 
 --- !COFF
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -379,6 +379,13 @@
   if (complex_type == llvm::COFF::IMAGE_SYM_DTYPE_FUNCTION) {
 return lldb::eSymbolTypeCode;
   }
+  const auto base_type = coff_symbol_type & 0xff;
+  if (base_type == llvm::COFF::IMAGE_SYM_TYPE_NULL &&
+  

[Lldb-commits] [PATCH] D134585: [lldb][COFF] Map symbols without base+complex type as 'Data' type

[David Spickett via Phabricator via lldb-commits]

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

Would help if I actually clicked the buttons yes :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134585

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

[Lldb-commits] [PATCH] D134585: [lldb][COFF] Map symbols without base+complex type as 'Data' type

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

In D134585#3814463 , @DavidSpickett 
wrote:

>> MSVC/link.exe doesn't write symbols into linked PE images at all.
>
> So by the time we get to a debugger, it's not an issue anyway.

Yep, most of these patches about symbol table handling doesn't make any 
difference for the MSVC ecosystem usage at all (although some of these patches 
fix other generic windows-specific bugs noticed too).

> Then this LGTM.

Thanks! Can you mark it formally approved too? :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134585

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

[Lldb-commits] [PATCH] D134585: [lldb][COFF] Map symbols without base+complex type as 'Data' type

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

> MSVC/link.exe doesn't write symbols into linked PE images at all.

So by the time we get to a debugger, it's not an issue anyway.

Then this LGTM.

Thanks for all these improvements btw, I expect Linaro's Windows on Arm efforts 
appreciate them too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134585

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

[Lldb-commits] [PATCH] D134585: [lldb][COFF] Map symbols without base+complex type as 'Data' type

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

In D134585#3814458 , @DavidSpickett 
wrote:

> You're the expert on what the linker does and the code looks fine.
>
> Is it possible that msvc uses these `IMAGE_SYM_TYPE_NULL` in a different way 
> that could cause a problem? Worst that happens is we get a bunch of symbols 
> available in expressions that shouldn't be?

MSVC/link.exe doesn't write symbols into linked PE images at all. But at the 
object file stage, they do express data symbols in the same way (which is the 
default marking for symbols that have no other types set).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134585

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

[Lldb-commits] [PATCH] D134585: [lldb][COFF] Map symbols without base+complex type as 'Data' type

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

You're the expert on what the linker does and the code looks fine.

Is it possible that msvc uses these `IMAGE_SYM_TYPE_NULL` in a different way 
that could cause a problem? Worst that happens is we get a bunch of symbols 
available in expressions that shouldn't be?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134585

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

[Lldb-commits] [PATCH] D134585: [lldb][COFF] Map symbols without base+complex type as 'Data' type

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

This LGTM, but I'll leave it to some LLDB maintainer to formally approve.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134585

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

[Lldb-commits] [PATCH] D134585: [lldb][COFF] Map symbols without base+complex type as 'Data' type

[Alvin Wong via Phabricator via lldb-commits]

alvinhochun created this revision.
alvinhochun added reviewers: labath, DavidSpickett, mstorsjo.
Herald added a project: All.
alvinhochun requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Both LLD and GNU ld write global/static variables to the COFF symbol
table with `IMAGE_SYM_TYPE_NULL` and `IMAGE_SYM_DTYPE_NULL` type. Map
these symbols as 'Data' type in the symtab to allow these symbols to be
used in expressions and printable.

Depends on D134426 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134585

Files:
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
  lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml


Index: lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
@@ -15,7 +15,7 @@
 # CHECK-NEXT: 4294967295 Code   0x000180001020
0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasFunc
 # CHECK-NEXT: 4294967295   X Additional 0x000180003000
0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
 # CHECK-NEXT: 4294967295 Data   0x000180003004
0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasInt
-# CHECK-NEXT: 4294967295 Invalid0x000180003008
0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} internalInt
+# CHECK-NEXT: 4294967295 Data   0x000180003008
0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} internalInt
 # CHECK-EMPTY:
 
 # Test file generated with:
Index: lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
@@ -6,7 +6,7 @@
 
 # CHECK: Type File Address/Value {{.*}} SizeFlags   
Name
 # CHECK: Code 0x400010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} 
entry
-# CHECK:  0x400020000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} 
variable
+# CHECK: Data 0x400020000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} 
variable
 # CHECK: Absolute 0xdeadbeef0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} 
absolute_symbol
 
 --- !COFF
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -379,6 +379,13 @@
   if (complex_type == llvm::COFF::IMAGE_SYM_DTYPE_FUNCTION) {
 return lldb::eSymbolTypeCode;
   }
+  const auto base_type = coff_symbol_type & 0xff;
+  if (base_type == llvm::COFF::IMAGE_SYM_TYPE_NULL &&
+  complex_type == llvm::COFF::IMAGE_SYM_DTYPE_NULL) {
+// Unknown type. LLD and GNU ld uses this for variables on MinGW, so
+// consider these symbols to be data to enable printing.
+return lldb::eSymbolTypeData;
+  }
   return lldb::eSymbolTypeInvalid;
 }
 


Index: lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/symbols-export-table.yaml
@@ -15,7 +15,7 @@
 # CHECK-NEXT: 4294967295 Code   0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasFunc
 # CHECK-NEXT: 4294967295   X Additional 0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
 # CHECK-NEXT: 4294967295 Data   0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasInt
-# CHECK-NEXT: 4294967295 Invalid0x0001800030080x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} internalInt
+# CHECK-NEXT: 4294967295 Data   0x0001800030080x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} internalInt
 # CHECK-EMPTY:
 
 # Test file generated with:
Index: lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
===
--- lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
+++ lldb/test/Shell/ObjectFile/PECOFF/symbol.yaml
@@ -6,7 +6,7 @@
 
 # CHECK: Type File Address/Value {{.*}} SizeFlags   Name
 # CHECK: Code 0x400010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
-# CHECK:  0x400020000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} variable
+# CHECK: Data 0x400020000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} variable
 # CHECK: Absolute 0xdeadbeef0x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} absolute_symbol
 
 --- !COFF
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -379,6 +379,13 @@
   if (complex_type ==