[Lldb-commits] [PATCH] D134426: [lldb][COFF] Match symbols from COFF symbol table to export symbols

2022-09-28 Thread Martin Storsjö via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG20c2f94c3cc1: [lldb][COFF] Match symbols from COFF symbol 
table to export symbols (authored by alvinhochun, committed by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134426

Files:
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
  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
@@ -4,18 +4,18 @@
 # Checks that the symtab contains both symbols from the export table and the
 # COFF symbol table.
 
-# CHECK:  UserID DSX TypeFile Address/Value {{.*}} SizeFlags   Name
+# CHECK:  UserID DSX Type   File Address/Value {{.*}} SizeFlags   Name
 # CHECK-NEXT: --
-# CHECK-NEXT:  1   X Code0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFnAlias
-# CHECK-NEXT:  2   X Code0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
-# CHECK-NEXT:  3   X Data0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
-# CHECK-NEXT:  4   X Data0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportIntAlias
-# CHECK-NEXT: 4294967295 Code0x0001800010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
-# CHECK-NEXT: 4294967295 Code0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
-# CHECK-NEXT: 4294967295 Code0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasFunc
-# CHECK-NEXT: 4294967295 Invalid 0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
-# CHECK-NEXT: 4294967295 Invalid 0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasInt
-# CHECK-NEXT: 4294967295 Invalid 0x0001800030080x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} internalInt
+# CHECK-NEXT:  1   X Code   0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFnAlias
+# CHECK-NEXT:  2   X Code   0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
+# CHECK-NEXT:  3   X Data   0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
+# CHECK-NEXT:  4   X Data   0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportIntAlias
+# CHECK-NEXT: 4294967295 Code   0x0001800010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
+# CHECK-NEXT: 4294967295   X Additional 0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
+# 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-EMPTY:
 
 # Test file generated with:
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
@@ -268,10 +268,12 @@
 
 private:
   bool CreateBinary();
+  typedef std::vector> rva_symbol_list_t;
   void AppendFromCOFFSymbolTable(lldb_private::SectionList *sect_list,
- lldb_private::Symtab );
-  void AppendFromExportTable(lldb_private::SectionList *sect_list,
- lldb_private::Symtab );
+ lldb_private::Symtab ,
+ const rva_symbol_list_t _exports);
+  rva_symbol_list_t AppendFromExportTable(lldb_private::SectionList *sect_list,
+  lldb_private::Symtab );
 
   dos_header_t m_dos_header;
   coff_header_t m_coff_header;
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -761,12 +761,18 @@
 
 void ObjectFilePECOFF::ParseSymtab(Symtab ) {
   SectionList *sect_list = GetSectionList();
-  AppendFromExportTable(sect_list, symtab);
-  AppendFromCOFFSymbolTable(sect_list, symtab);
+  rva_symbol_list_t sorted_exports = AppendFromExportTable(sect_list, symtab);
+  AppendFromCOFFSymbolTable(sect_list, symtab, sorted_exports);
 }
 
-void

[Lldb-commits] [PATCH] D134426: [lldb][COFF] Match symbols from COFF symbol table to export symbols

2022-09-27 Thread Pavel Labath via Phabricator via lldb-commits 
labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

Seems fine to me from a general perspective. I think others have already 
checked the windows parts.




Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:892
   }
+  std::sort(export_list.begin(), export_list.end(), RVASymbolListCompareRVA);
+  return export_list;

alvinhochun wrote:
> labath wrote:
> > Can you have multiple symbols pointing to the same address? Make this 
> > should use `stable_sort` instead?
> Yes, it can happen. The ordering shouldn't affect what 
> AppendFromCOFFSymbolTable does but I suppose stable_sort does make it more 
> deterministic to avoid future issues down the line.
Yeah, it's better to avoid having this kind of nondeterministic behavior that 
can differ from run to run. LLDB is not so string about it as clang/llvm, but 
it's still a good idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134426

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

[Lldb-commits] [PATCH] D134426: [lldb][COFF] Match symbols from COFF symbol table to export symbols

2022-09-23 Thread Martin Storsjö via Phabricator via lldb-commits 
mstorsjo added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:828
+  // either names will work. Only synchronize the symbol type.
+  if (symbol.GetType() == lldb::eSymbolTypeInvalid)
+symbol.SetType(exported->GetType());

alvinhochun wrote:
> mstorsjo wrote:
> > alvinhochun wrote:
> > > mstorsjo wrote:
> > > > This condition had me puzzled for a moment, until I realized that 
> > > > you're synchronizing symbol type in the other direction here. Is it 
> > > > worth clarifying this in the comment?
> > > > 
> > > > (Also, I presume the test case doesn't really trigger this case?)
> > > Hmm, I can try to improve the comment.
> > > 
> > > The `aliasInt` symbol should trigger this case. Perhaps it will be 
> > > clearer if I update the previous patch to include these symbols, so the 
> > > difference in output can be seen in this patch.
> > Oh, ok, I had missed that `MapSymbolType` returns code or invalid - I had 
> > expected it to return code or data. I guess this is fine then. (Changing 
> > that function, or changing the logic here to look at section types is out 
> > of scope here anyway, and I don’t know if the difference between data and 
> > invalid matters?)
> Data vs Invalid does seem to have an effect on whether a symbol can be 
> printed as data or have its address taken. I can't print or dump any 
> static/globals when the type is Invalid. I think the DWARF debug info should 
> have at least some information about these symbols, but I have no idea how 
> that works.
> 
> I wonder if I should change `MapSymbolType` to return Data for anything that 
> is not code?
I think that might be a good idea - but that's clearly a separate change from 
this one too. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134426

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

[Lldb-commits] [PATCH] D134426: [lldb][COFF] Match symbols from COFF symbol table to export symbols

2022-09-23 Thread Alvin Wong via Phabricator via lldb-commits 
alvinhochun added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:828
+  // either names will work. Only synchronize the symbol type.
+  if (symbol.GetType() == lldb::eSymbolTypeInvalid)
+symbol.SetType(exported->GetType());

mstorsjo wrote:
> alvinhochun wrote:
> > mstorsjo wrote:
> > > This condition had me puzzled for a moment, until I realized that you're 
> > > synchronizing symbol type in the other direction here. Is it worth 
> > > clarifying this in the comment?
> > > 
> > > (Also, I presume the test case doesn't really trigger this case?)
> > Hmm, I can try to improve the comment.
> > 
> > The `aliasInt` symbol should trigger this case. Perhaps it will be clearer 
> > if I update the previous patch to include these symbols, so the difference 
> > in output can be seen in this patch.
> Oh, ok, I had missed that `MapSymbolType` returns code or invalid - I had 
> expected it to return code or data. I guess this is fine then. (Changing that 
> function, or changing the logic here to look at section types is out of scope 
> here anyway, and I don’t know if the difference between data and invalid 
> matters?)
Data vs Invalid does seem to have an effect on whether a symbol can be printed 
as data or have its address taken. I can't print or dump any static/globals 
when the type is Invalid. I think the DWARF debug info should have at least 
some information about these symbols, but I have no idea how that works.

I wonder if I should change `MapSymbolType` to return Data for anything that is 
not code?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134426

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

[Lldb-commits] [PATCH] D134426: [lldb][COFF] Match symbols from COFF symbol table to export symbols

2022-09-23 Thread Martin Storsjö via Phabricator via lldb-commits 
mstorsjo added a comment.

No further objections/comments from me on this!




Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:828
+  // either names will work. Only synchronize the symbol type.
+  if (symbol.GetType() == lldb::eSymbolTypeInvalid)
+symbol.SetType(exported->GetType());

alvinhochun wrote:
> mstorsjo wrote:
> > This condition had me puzzled for a moment, until I realized that you're 
> > synchronizing symbol type in the other direction here. Is it worth 
> > clarifying this in the comment?
> > 
> > (Also, I presume the test case doesn't really trigger this case?)
> Hmm, I can try to improve the comment.
> 
> The `aliasInt` symbol should trigger this case. Perhaps it will be clearer if 
> I update the previous patch to include these symbols, so the difference in 
> output can be seen in this patch.
Oh, ok, I had missed that `MapSymbolType` returns code or invalid - I had 
expected it to return code or data. I guess this is fine then. (Changing that 
function, or changing the logic here to look at section types is out of scope 
here anyway, and I don’t know if the difference between data and invalid 
matters?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134426

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

[Lldb-commits] [PATCH] D134426: [lldb][COFF] Match symbols from COFF symbol table to export symbols

2022-09-23 Thread Alvin Wong via Phabricator via lldb-commits 
alvinhochun updated this revision to Diff 462476.
alvinhochun added a comment.

Updated test based on parent and addressed review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134426

Files:
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
  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
@@ -4,18 +4,18 @@
 # Checks that the symtab contains both symbols from the export table and the
 # COFF symbol table.
 
-# CHECK:  UserID DSX TypeFile Address/Value {{.*}} SizeFlags   Name
+# CHECK:  UserID DSX Type   File Address/Value {{.*}} SizeFlags   Name
 # CHECK-NEXT: --
-# CHECK-NEXT:  1   X Code0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFnAlias
-# CHECK-NEXT:  2   X Code0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
-# CHECK-NEXT:  3   X Data0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
-# CHECK-NEXT:  4   X Data0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportIntAlias
-# CHECK-NEXT: 4294967295 Code0x0001800010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
-# CHECK-NEXT: 4294967295 Code0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
-# CHECK-NEXT: 4294967295 Code0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasFunc
-# CHECK-NEXT: 4294967295 Invalid 0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
-# CHECK-NEXT: 4294967295 Invalid 0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} aliasInt
-# CHECK-NEXT: 4294967295 Invalid 0x0001800030080x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} internalInt
+# CHECK-NEXT:  1   X Code   0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFnAlias
+# CHECK-NEXT:  2   X Code   0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
+# CHECK-NEXT:  3   X Data   0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
+# CHECK-NEXT:  4   X Data   0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportIntAlias
+# CHECK-NEXT: 4294967295 Code   0x0001800010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
+# CHECK-NEXT: 4294967295   X Additional 0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
+# 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-EMPTY:
 
 # Test file generated with:
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
@@ -268,10 +268,12 @@
 
 private:
   bool CreateBinary();
+  typedef std::vector> rva_symbol_list_t;
   void AppendFromCOFFSymbolTable(lldb_private::SectionList *sect_list,
- lldb_private::Symtab );
-  void AppendFromExportTable(lldb_private::SectionList *sect_list,
- lldb_private::Symtab );
+ lldb_private::Symtab ,
+ const rva_symbol_list_t _exports);
+  rva_symbol_list_t AppendFromExportTable(lldb_private::SectionList *sect_list,
+  lldb_private::Symtab );
 
   dos_header_t m_dos_header;
   coff_header_t m_coff_header;
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -761,12 +761,18 @@
 
 void ObjectFilePECOFF::ParseSymtab(Symtab ) {
   SectionList *sect_list = GetSectionList();
-  AppendFromExportTable(sect_list, symtab);
-  AppendFromCOFFSymbolTable(sect_list, symtab);
+  rva_symbol_list_t sorted_exports = AppendFromExportTable(sect_list, symtab);
+  AppendFromCOFFSymbolTable(sect_list, symtab, sorted_exports);
 }
 
-void ObjectFilePECOFF::AppendFromCOFFSymbolTable(SectionList *sect_list,
- Symtab )

[Lldb-commits] [PATCH] D134426: [lldb][COFF] Match symbols from COFF symbol table to export symbols

2022-09-23 Thread Alvin Wong via Phabricator via lldb-commits 
alvinhochun added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:817
+if (symbol_type != lldb::eSymbolTypeInvalid)
+  exported->SetType(symbol_type);
+if (exported->GetMangled() == symbol.GetMangled()) {

mstorsjo wrote:
> As a curious question - since D134265 we get better quality for the symbol 
> types set from the get go - what are the other cases you foresee where this 
> will make a difference? I don't mind if there isn't any difference though, 
> syncing the types from the symbol table which is more expressible probably is 
> good anyway. Just wondering about the actual utility of it.
Given what limited info the existing implementations (LLD and GNU ld) write to 
the symbol table, we probably won't get any better type info from just the 
symbol table alone. Though I was thinking if we can use a bit of additional 
heuristics we can assign more specific symbol types.

For example, perhaps if we can guess that a particular code symbol may be an 
import thunk from the jump instruction it contains, then we can set the symbol 
as 'eSymbolTypeTrampoline'. It seems the breakpoint command will skip 
trampoline symbols, so if you, say, set a breakpoint on `memcpy` it will only 
break on the real function but not the import thunk. Not sure how feasible it 
is to implement though.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:828
+  // either names will work. Only synchronize the symbol type.
+  if (symbol.GetType() == lldb::eSymbolTypeInvalid)
+symbol.SetType(exported->GetType());

mstorsjo wrote:
> This condition had me puzzled for a moment, until I realized that you're 
> synchronizing symbol type in the other direction here. Is it worth clarifying 
> this in the comment?
> 
> (Also, I presume the test case doesn't really trigger this case?)
Hmm, I can try to improve the comment.

The `aliasInt` symbol should trigger this case. Perhaps it will be clearer if I 
update the previous patch to include these symbols, so the difference in output 
can be seen in this patch.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:892
   }
+  std::sort(export_list.begin(), export_list.end(), RVASymbolListCompareRVA);
+  return export_list;

labath wrote:
> Can you have multiple symbols pointing to the same address? Make this should 
> use `stable_sort` instead?
Yes, it can happen. The ordering shouldn't affect what 
AppendFromCOFFSymbolTable does but I suppose stable_sort does make it more 
deterministic to avoid future issues down the line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134426

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

[Lldb-commits] [PATCH] D134426: [lldb][COFF] Match symbols from COFF symbol table to export symbols

2022-09-23 Thread Pavel Labath via Phabricator via lldb-commits 
labath added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:892
   }
+  std::sort(export_list.begin(), export_list.end(), RVASymbolListCompareRVA);
+  return export_list;

Can you have multiple symbols pointing to the same address? Make this should 
use `stable_sort` instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134426

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

[Lldb-commits] [PATCH] D134426: [lldb][COFF] Match symbols from COFF symbol table to export symbols

2022-09-23 Thread Martin Storsjö via Phabricator via lldb-commits 
mstorsjo added a comment.
Herald added a subscriber: JDevlieghere.

This looks mostly reasonable to me, a couple discussion points only.




Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:817
+if (symbol_type != lldb::eSymbolTypeInvalid)
+  exported->SetType(symbol_type);
+if (exported->GetMangled() == symbol.GetMangled()) {

As a curious question - since D134265 we get better quality for the symbol 
types set from the get go - what are the other cases you foresee where this 
will make a difference? I don't mind if there isn't any difference though, 
syncing the types from the symbol table which is more expressible probably is 
good anyway. Just wondering about the actual utility of it.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:828
+  // either names will work. Only synchronize the symbol type.
+  if (symbol.GetType() == lldb::eSymbolTypeInvalid)
+symbol.SetType(exported->GetType());

This condition had me puzzled for a moment, until I realized that you're 
synchronizing symbol type in the other direction here. Is it worth clarifying 
this in the comment?

(Also, I presume the test case doesn't really trigger this case?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134426

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

[Lldb-commits] [PATCH] D134426: [lldb][COFF] Match symbols from COFF symbol table to export symbols

2022-09-22 Thread Alvin Wong via Phabricator via lldb-commits 
alvinhochun created this revision.
alvinhochun added reviewers: labath, DavidSpickett, mstorsjo.
Herald added a subscriber: mgrang.
Herald added a project: All.
alvinhochun published this revision for review.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

If a symbol is the same as an export symbol, mark it as 'Additional' to
prevent the duplicated symbol from being repeated in some commands (e.g.
`disas -n func`). If the RVA is the same but exported with a different
name, only synchronize the symbol types.

Depends on D134265 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134426

Files:
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
  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
@@ -6,20 +6,28 @@
 
 # CHECK:  UserID DSX TypeFile Address/Value {{.*}} SizeFlags   Name
 # CHECK-NEXT: --
-# CHECK-NEXT:  1   X Code0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
-# CHECK-NEXT:  2   X Data0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
-# CHECK-NEXT: 4294967295 Code0x0001800010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
-# CHECK-NEXT: 4294967295 Code0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
-# CHECK-NEXT: 4294967295 Invalid 0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
+# CHECK-NEXT:  1   X Code   0x0001800010200x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFnAlias
+# CHECK-NEXT:  2   X Code   0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
+# CHECK-NEXT:  3   X Data   0x0001800030000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportInt
+# CHECK-NEXT:  4   X Data   0x0001800030040x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportIntAlias
+# CHECK-NEXT: 4294967295 Code   0x0001800010000x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} entry
+# CHECK-NEXT: 4294967295   X Additional 0x0001800010100x{{[0-9a-f]+}} 0x{{[0-9a-f]+}} exportFunc
+# 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-EMPTY:
 
 # Test file generated with:
 #   clang -O2 --target=x86_64-windows-gnu test.c -nostdlib -c -o test.obj
-#   lld-link -lldmingw -debug:symtab -dll -out:test.dll -entry:entry test.obj
+#   lld-link -lldmingw -debug:symtab -dll -out:test.dll -entry:entry -export:exportFnAlias=aliasFn -export:exportIntAlias=aliasInt test.obj
 # test.c:
 #   __attribute__((dllexport)) int exportInt;
+#   int internalInt;
+#   int aliasInt;
 #   void entry(void) {}
 #   __attribute__((dllexport)) void exportFunc(void) {}
+#   void aliasFunc(void) {}
 
 --- !COFF
 OptionalHeader:
@@ -41,7 +49,7 @@
   SizeOfHeapCommit: 4096
   ExportTable:
 RelativeVirtualAddress: 8224
-Size:107
+Size:156
 header:
   Machine: IMAGE_FILE_MACHINE_AMD64
   Characteristics: [ IMAGE_FILE_EXECUTABLE_IMAGE, IMAGE_FILE_LARGE_ADDRESS_AWARE, IMAGE_FILE_DLL ]
@@ -49,17 +57,17 @@
   - Name:.text
 Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
 VirtualAddress:  4096
-VirtualSize: 17
-SectionData: C32E0F1F8400C3
+VirtualSize: 33
+SectionData: C32E0F1F8400C32E0F1F8400C3
   - Name:.rdata
 Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ ]
 VirtualAddress:  8192
-VirtualSize: 139
-SectionData: 482001000200020062206A20722073796D626F6C732D6578706F7274732E632E746D702E646C6C00101000307620812001006578706F727446756E63006578706F7274496E7400
+VirtualSize: 188
+SectionData: