[Lldb-commits] [PATCH] D62772: [COFF, ARM64] Fix CodeView API change for getRegisterNames

[Tom Tan via Phabricator via lldb-commits]

TomTan added a comment.

In D62772#1526824 , @compnerd wrote:

> This should get the build working again, so lets get this fixed, we can 
> improve it later


Thanks, committed the fix to unblock build. For WoA(ARM32) which is not 
supported, I meant LLVM CodeView, not CodeView specification. 
`llvm::codeview::getRegisterNames(cpu_type)` will return incorrect register 
name even the right `CPUType` is passed in for WoA(ARM32), because it has not 
been implemented in LLVM CodeView yet.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62772



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

[Lldb-commits] [PATCH] D62772: [COFF, ARM64] Fix CodeView API change for getRegisterNames

[Tom Tan via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL362349: [COFF, ARM64] Fix CodeView API change for 
getRegisterNames (authored by TomTan, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D62772?vs=202614&id=202628#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D62772

Files:
  
lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp


Index: 
lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
===
--- 
lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
+++ 
lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
@@ -25,8 +25,19 @@
 
 static uint32_t ResolveLLDBRegisterNum(llvm::StringRef reg_name, 
llvm::Triple::ArchType arch_type) {
   // lookup register name to get lldb register number
+  llvm::codeview::CPUType cpu_type;
+  switch (arch_type) {
+case llvm::Triple::ArchType::aarch64:
+  cpu_type = llvm::codeview::CPUType::ARM64;
+  break;
+
+default:
+  cpu_type = llvm::codeview::CPUType::X64;
+  break;
+  }
+
   llvm::ArrayRef> register_names =
-  llvm::codeview::getRegisterNames();
+  llvm::codeview::getRegisterNames(cpu_type);
   auto it = llvm::find_if(
   register_names,
   [®_name](const llvm::EnumEntry ®ister_entry) {


Index: lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
===
--- lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
+++ lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
@@ -25,8 +25,19 @@
 
 static uint32_t ResolveLLDBRegisterNum(llvm::StringRef reg_name, llvm::Triple::ArchType arch_type) {
   // lookup register name to get lldb register number
+  llvm::codeview::CPUType cpu_type;
+  switch (arch_type) {
+case llvm::Triple::ArchType::aarch64:
+  cpu_type = llvm::codeview::CPUType::ARM64;
+  break;
+
+default:
+  cpu_type = llvm::codeview::CPUType::X64;
+  break;
+  }
+
   llvm::ArrayRef> register_names =
-  llvm::codeview::getRegisterNames();
+  llvm::codeview::getRegisterNames(cpu_type);
   auto it = llvm::find_if(
   register_names,
   [®_name](const llvm::EnumEntry ®ister_entry) {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D62772: [COFF, ARM64] Fix CodeView API change for getRegisterNames

[Saleem Abdulrasool via Phabricator via lldb-commits]

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

This should get the build working again, so lets get this fixed, we can improve 
it later




Comment at: 
source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:29
+  llvm::codeview::CPUType cpu_type;
+  if (arch_type == llvm::Triple::ArchType::aarch64) {
+cpu_type = llvm::codeview::CPUType::ARM64;

TomTan wrote:
> TomTan wrote:
> > compnerd wrote:
> > > aganea wrote:
> > > > Shouldn’t ArchType::aarch64_be and ArchType::aarch64_32 enums be 
> > > > handled here as well?
> > > I think that we should use a `switch` to cover the targets.  `/Oy` will 
> > > allow FPO on x86 as well.  There is also WoA (ARM32).
> > Seems no, aarch64_be or aarch64_32 is not supported by CodeView or Windows.
> Ok. It makes sense to use switch/case. CodeView doesn't support WoA (ARM32) 
> so no need to add case for it here.
Huh?  How does `cl` generate that then?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D62772



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

[Lldb-commits] [PATCH] D62772: [COFF, ARM64] Fix CodeView API change for getRegisterNames

[Tom Tan via Phabricator via lldb-commits]

TomTan marked an inline comment as done.
TomTan added inline comments.



Comment at: 
source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:29
+  llvm::codeview::CPUType cpu_type;
+  if (arch_type == llvm::Triple::ArchType::aarch64) {
+cpu_type = llvm::codeview::CPUType::ARM64;

TomTan wrote:
> compnerd wrote:
> > aganea wrote:
> > > Shouldn’t ArchType::aarch64_be and ArchType::aarch64_32 enums be handled 
> > > here as well?
> > I think that we should use a `switch` to cover the targets.  `/Oy` will 
> > allow FPO on x86 as well.  There is also WoA (ARM32).
> Seems no, aarch64_be or aarch64_32 is not supported by CodeView or Windows.
Ok. It makes sense to use switch/case. CodeView doesn't support WoA (ARM32) so 
no need to add case for it here.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D62772



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

[Lldb-commits] [PATCH] D62772: [COFF, ARM64] Fix CodeView API change for getRegisterNames

[Tom Tan via Phabricator via lldb-commits]

TomTan updated this revision to Diff 202614.
TomTan added a comment.

Change to switch/case based on comment.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D62772

Files:
  source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp


Index: source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
===
--- source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
+++ source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
@@ -25,8 +25,19 @@
 
 static uint32_t ResolveLLDBRegisterNum(llvm::StringRef reg_name, 
llvm::Triple::ArchType arch_type) {
   // lookup register name to get lldb register number
+  llvm::codeview::CPUType cpu_type;
+  switch (arch_type) {
+case llvm::Triple::ArchType::aarch64:
+  cpu_type = llvm::codeview::CPUType::ARM64;
+  break;
+
+default:
+  cpu_type = llvm::codeview::CPUType::X64;
+  break;
+  }
+
   llvm::ArrayRef> register_names =
-  llvm::codeview::getRegisterNames();
+  llvm::codeview::getRegisterNames(cpu_type);
   auto it = llvm::find_if(
   register_names,
   [®_name](const llvm::EnumEntry ®ister_entry) {


Index: source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
===
--- source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
+++ source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
@@ -25,8 +25,19 @@
 
 static uint32_t ResolveLLDBRegisterNum(llvm::StringRef reg_name, llvm::Triple::ArchType arch_type) {
   // lookup register name to get lldb register number
+  llvm::codeview::CPUType cpu_type;
+  switch (arch_type) {
+case llvm::Triple::ArchType::aarch64:
+  cpu_type = llvm::codeview::CPUType::ARM64;
+  break;
+
+default:
+  cpu_type = llvm::codeview::CPUType::X64;
+  break;
+  }
+
   llvm::ArrayRef> register_names =
-  llvm::codeview::getRegisterNames();
+  llvm::codeview::getRegisterNames(cpu_type);
   auto it = llvm::find_if(
   register_names,
   [®_name](const llvm::EnumEntry ®ister_entry) {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D62772: [COFF, ARM64] Fix CodeView API change for getRegisterNames

[Tom Tan via Phabricator via lldb-commits]

TomTan marked an inline comment as done.
TomTan added inline comments.



Comment at: 
source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:29
+  llvm::codeview::CPUType cpu_type;
+  if (arch_type == llvm::Triple::ArchType::aarch64) {
+cpu_type = llvm::codeview::CPUType::ARM64;

compnerd wrote:
> aganea wrote:
> > Shouldn’t ArchType::aarch64_be and ArchType::aarch64_32 enums be handled 
> > here as well?
> I think that we should use a `switch` to cover the targets.  `/Oy` will allow 
> FPO on x86 as well.  There is also WoA (ARM32).
Seems no, aarch64_be or aarch64_32 is not supported by CodeView or Windows.



Comment at: 
source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:32
+  } else {
+cpu_type = llvm::codeview::CPUType::X64;
+  }

aganea wrote:
> Intel80386 for consistency?
x86 and x64 are handled as the same `CPUType` by CodeView. LLVM pdb dump tool 
also sets X64 as default.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D62772



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

[Lldb-commits] [PATCH] D62772: [COFF, ARM64] Fix CodeView API change for getRegisterNames

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd requested changes to this revision.
compnerd added inline comments.
This revision now requires changes to proceed.



Comment at: 
source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:29
+  llvm::codeview::CPUType cpu_type;
+  if (arch_type == llvm::Triple::ArchType::aarch64) {
+cpu_type = llvm::codeview::CPUType::ARM64;

aganea wrote:
> Shouldn’t ArchType::aarch64_be and ArchType::aarch64_32 enums be handled here 
> as well?
I think that we should use a `switch` to cover the targets.  `/Oy` will allow 
FPO on x86 as well.  There is also WoA (ARM32).


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D62772



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

[Lldb-commits] [PATCH] D62772: [COFF, ARM64] Fix CodeView API change for getRegisterNames

[Alexandre Ganea via Phabricator via lldb-commits]

aganea added inline comments.



Comment at: 
source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:29
+  llvm::codeview::CPUType cpu_type;
+  if (arch_type == llvm::Triple::ArchType::aarch64) {
+cpu_type = llvm::codeview::CPUType::ARM64;

Shouldn’t ArchType::aarch64_be and ArchType::aarch64_32 enums be handled here 
as well?



Comment at: 
source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:32
+  } else {
+cpu_type = llvm::codeview::CPUType::X64;
+  }

Intel80386 for consistency?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D62772



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

[Lldb-commits] [PATCH] D62772: [COFF, ARM64] Fix CodeView API change for getRegisterNames

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

D62771  does address the same.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D62772



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

[Lldb-commits] [PATCH] D62772: [COFF, ARM64] Fix CodeView API change for getRegisterNames

[Tom Tan via Phabricator via lldb-commits]

TomTan added a comment.

In D62772#1526145 , @mgorny wrote:

> Are you sure this triple → CPUType mapping belongs in each consumer? Maybe 
> it'd be better to have something inside LLVM, so that we wouldn't have to 
> keep this up-to-date in all the places. Maybe `getRegisterNames()` overload 
> that takes a triple?


The CodeView APIs seems not aware of LLVM tripe. `CPUType` comes from CodeView 
which makes this API self-consistent.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D62772



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

[Lldb-commits] [PATCH] D62772: [COFF, ARM64] Fix CodeView API change for getRegisterNames

[Tom Tan via Phabricator via lldb-commits]

TomTan updated this revision to Diff 202540.
TomTan edited the summary of this revision.
TomTan added a comment.

Update variable naming to be consistent with current file.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D62772

Files:
  source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp


Index: source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
===
--- source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
+++ source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
@@ -25,8 +25,14 @@
 
 static uint32_t ResolveLLDBRegisterNum(llvm::StringRef reg_name, 
llvm::Triple::ArchType arch_type) {
   // lookup register name to get lldb register number
+  llvm::codeview::CPUType cpu_type;
+  if (arch_type == llvm::Triple::ArchType::aarch64) {
+cpu_type = llvm::codeview::CPUType::ARM64;
+  } else {
+cpu_type = llvm::codeview::CPUType::X64;
+  }
   llvm::ArrayRef> register_names =
-  llvm::codeview::getRegisterNames();
+  llvm::codeview::getRegisterNames(cpu_type);
   auto it = llvm::find_if(
   register_names,
   [®_name](const llvm::EnumEntry ®ister_entry) {


Index: source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
===
--- source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
+++ source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
@@ -25,8 +25,14 @@
 
 static uint32_t ResolveLLDBRegisterNum(llvm::StringRef reg_name, llvm::Triple::ArchType arch_type) {
   // lookup register name to get lldb register number
+  llvm::codeview::CPUType cpu_type;
+  if (arch_type == llvm::Triple::ArchType::aarch64) {
+cpu_type = llvm::codeview::CPUType::ARM64;
+  } else {
+cpu_type = llvm::codeview::CPUType::X64;
+  }
   llvm::ArrayRef> register_names =
-  llvm::codeview::getRegisterNames();
+  llvm::codeview::getRegisterNames(cpu_type);
   auto it = llvm::find_if(
   register_names,
   [®_name](const llvm::EnumEntry ®ister_entry) {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D62772: [COFF, ARM64] Fix CodeView API change for getRegisterNames

[Michał Górny via Phabricator via lldb-commits]

mgorny added a reviewer: labath.
mgorny added a comment.

Are you sure this triple → CPUType mapping belongs in each consumer? Maybe it'd 
be better to have something inside LLVM, so that we wouldn't have to keep this 
up-to-date in all the places. Maybe `getRegisterNames()` overload that takes a 
triple?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D62772



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

[Lldb-commits] [PATCH] D62772: [COFF, ARM64] Fix CodeView API change for getRegisterNames

[Tom Tan via Phabricator via lldb-commits]

TomTan created this revision.
TomTan added reviewers: efriedma, rnk, mgorny, mstorsjo.
TomTan added a project: LLDB.
Herald added subscribers: lldb-commits, kristof.beyls.

Change rL362280  
(https://reviews.llvm.org/rL362280) changed CodeView API getRegisterNames() by 
adding an input parameter in CPUType. It is called in LLDB and needs to be 
updated.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D62772

Files:
  source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp


Index: source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
===
--- source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
+++ source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
@@ -25,8 +25,14 @@
 
 static uint32_t ResolveLLDBRegisterNum(llvm::StringRef reg_name, 
llvm::Triple::ArchType arch_type) {
   // lookup register name to get lldb register number
+  llvm::codeview::CPUType Cpu;
+  if (arch_type == llvm::Triple::ArchType::aarch64) {
+Cpu = llvm::codeview::CPUType::ARM64;
+  } else {
+Cpu = llvm::codeview::CPUType::X64;
+  }
   llvm::ArrayRef> register_names =
-  llvm::codeview::getRegisterNames();
+  llvm::codeview::getRegisterNames(Cpu);
   auto it = llvm::find_if(
   register_names,
   [®_name](const llvm::EnumEntry ®ister_entry) {


Index: source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
===
--- source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
+++ source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
@@ -25,8 +25,14 @@
 
 static uint32_t ResolveLLDBRegisterNum(llvm::StringRef reg_name, llvm::Triple::ArchType arch_type) {
   // lookup register name to get lldb register number
+  llvm::codeview::CPUType Cpu;
+  if (arch_type == llvm::Triple::ArchType::aarch64) {
+Cpu = llvm::codeview::CPUType::ARM64;
+  } else {
+Cpu = llvm::codeview::CPUType::X64;
+  }
   llvm::ArrayRef> register_names =
-  llvm::codeview::getRegisterNames();
+  llvm::codeview::getRegisterNames(Cpu);
   auto it = llvm::find_if(
   register_names,
   [®_name](const llvm::EnumEntry ®ister_entry) {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits