[Lldb-commits] [PATCH] D27380: [lldb] Update the check for Linux or FreeBSD in SymbolFileDWARF::FindFunctions.

[Alexander Shaposhnikov via Phabricator via lldb-commits]

alexshap updated this revision to Diff 80278.
alexshap added a comment.

Address comments


Repository:
  rL LLVM

https://reviews.llvm.org/D27380

Files:
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp


Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -2586,8 +2586,7 @@
   if (sc_list.GetSize() == original_size) {
 ArchSpec arch;
 if (!parent_decl_ctx && GetObjectFile()->GetArchitecture(arch) &&
-(arch.GetTriple().isOSFreeBSD() || arch.GetTriple().isOSLinux() ||
- arch.GetMachine() == llvm::Triple::hexagon)) {
+arch.GetTriple().isOSBinFormatELF()) {
   SymbolContextList temp_sc_list;
   FindFunctions(name, m_function_basename_index, include_inlines,
 temp_sc_list);
Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1513,8 +1513,14 @@
 const uint32_t sub_type = subTypeFromElfHeader(header);
 arch_spec.SetArchitecture(eArchTypeELF, header.e_machine, sub_type,
   header.e_ident[EI_OSABI]);
-//
-// Validate if it is ok to remove GetOsFromOSABI
+
+// Validate if it is ok to remove GetOsFromOSABI.
+// Note, that now the OS is determined based on EI_OSABI flag and
+// the info extracted from ELF notes (see RefineModuleDetailsFromNote).
+// However in some cases that still might be not enough: for example
+// a shared library might not have any notes at all
+// and have EI_OSABI flag set to System V,
+// as result the OS will be set to UnknownOS.
 GetOsFromOSABI(header.e_ident[EI_OSABI], ostype);
 spec_ostype = arch_spec.GetTriple().getOS();
 assert(spec_ostype == ostype);


Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -2586,8 +2586,7 @@
   if (sc_list.GetSize() == original_size) {
 ArchSpec arch;
 if (!parent_decl_ctx && GetObjectFile()->GetArchitecture(arch) &&
-(arch.GetTriple().isOSFreeBSD() || arch.GetTriple().isOSLinux() ||
- arch.GetMachine() == llvm::Triple::hexagon)) {
+arch.GetTriple().isOSBinFormatELF()) {
   SymbolContextList temp_sc_list;
   FindFunctions(name, m_function_basename_index, include_inlines,
 temp_sc_list);
Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1513,8 +1513,14 @@
 const uint32_t sub_type = subTypeFromElfHeader(header);
 arch_spec.SetArchitecture(eArchTypeELF, header.e_machine, sub_type,
   header.e_ident[EI_OSABI]);
-//
-// Validate if it is ok to remove GetOsFromOSABI
+
+// Validate if it is ok to remove GetOsFromOSABI.
+// Note, that now the OS is determined based on EI_OSABI flag and
+// the info extracted from ELF notes (see RefineModuleDetailsFromNote).
+// However in some cases that still might be not enough: for example
+// a shared library might not have any notes at all
+// and have EI_OSABI flag set to System V,
+// as result the OS will be set to UnknownOS.
 GetOsFromOSABI(header.e_ident[EI_OSABI], ostype);
 spec_ostype = arch_spec.GetTriple().getOS();
 assert(spec_ostype == ostype);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D27380: [lldb] Update the check for Linux or FreeBSD in SymbolFileDWARF::FindFunctions.

[Pavel Labath via Phabricator via lldb-commits]

labath added a subscriber: ted.
labath added a comment.

It looks reasonable to me, but I don't know the full context, so I'd wait until 
others can comment on this as well.




Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2590
+(arch.GetTriple().isOSBinFormatELF() ||
  arch.GetMachine() == llvm::Triple::hexagon)) {
   SymbolContextList temp_sc_list;

I think we can remove the hexagon part as well. Ted, I presume hexagon uses ELF 
files?


Repository:
  rL LLVM

https://reviews.llvm.org/D27380



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

[Lldb-commits] [PATCH] D27380: [lldb] Update the check for Linux or FreeBSD in SymbolFileDWARF::FindFunctions.

[Alexander Shaposhnikov via Phabricator via lldb-commits]

alexshap created this revision.
alexshap added reviewers: clayborg, spyffe.
alexshap added a subscriber: lldb-commits.
alexshap set the repository for this revision to rL LLVM.

This diff

1. Adds a comment to ObjectFileELF.cpp about the current

approach to determining the OS.

2. Replaces the check in SymbolFileDWARF.cpp with a more robust one.

Test plan:

1. Checked that the build is green.
2. Built (on Linux) a test binary linked to a c++ shared library

which contains just an implementation of a function TestFunction,
the library (the binary itself) doesn't contain ELF notes and EI_OSABI is set 
to System V.
Checked in lldb that now "p TestFunction()" works fine (and doesn't work 
without this patch).


Repository:
  rL LLVM

https://reviews.llvm.org/D27380

Files:
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp


Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -2586,7 +2586,7 @@
   if (sc_list.GetSize() == original_size) {
 ArchSpec arch;
 if (!parent_decl_ctx && GetObjectFile()->GetArchitecture(arch) &&
-(arch.GetTriple().isOSFreeBSD() || arch.GetTriple().isOSLinux() ||
+(arch.GetTriple().isOSBinFormatELF() ||
  arch.GetMachine() == llvm::Triple::hexagon)) {
   SymbolContextList temp_sc_list;
   FindFunctions(name, m_function_basename_index, include_inlines,
Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1513,8 +1513,14 @@
 const uint32_t sub_type = subTypeFromElfHeader(header);
 arch_spec.SetArchitecture(eArchTypeELF, header.e_machine, sub_type,
   header.e_ident[EI_OSABI]);
-//
-// Validate if it is ok to remove GetOsFromOSABI
+
+// Validate if it is ok to remove GetOsFromOSABI.
+// Note, that now the OS is determined based on EI_OSABI flag and
+// the info extracted from ELF notes (see RefineModuleDetailsFromNote).
+// However in some cases that still might be not enough: for example
+// a shared library might not have any notes at all
+// and have EI_OSABI flag set to System V,
+// as result the OS will be set to UnknownOS.
 GetOsFromOSABI(header.e_ident[EI_OSABI], ostype);
 spec_ostype = arch_spec.GetTriple().getOS();
 assert(spec_ostype == ostype);


Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -2586,7 +2586,7 @@
   if (sc_list.GetSize() == original_size) {
 ArchSpec arch;
 if (!parent_decl_ctx && GetObjectFile()->GetArchitecture(arch) &&
-(arch.GetTriple().isOSFreeBSD() || arch.GetTriple().isOSLinux() ||
+(arch.GetTriple().isOSBinFormatELF() ||
  arch.GetMachine() == llvm::Triple::hexagon)) {
   SymbolContextList temp_sc_list;
   FindFunctions(name, m_function_basename_index, include_inlines,
Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1513,8 +1513,14 @@
 const uint32_t sub_type = subTypeFromElfHeader(header);
 arch_spec.SetArchitecture(eArchTypeELF, header.e_machine, sub_type,
   header.e_ident[EI_OSABI]);
-//
-// Validate if it is ok to remove GetOsFromOSABI
+
+// Validate if it is ok to remove GetOsFromOSABI.
+// Note, that now the OS is determined based on EI_OSABI flag and
+// the info extracted from ELF notes (see RefineModuleDetailsFromNote).
+// However in some cases that still might be not enough: for example
+// a shared library might not have any notes at all
+// and have EI_OSABI flag set to System V,
+// as result the OS will be set to UnknownOS.
 GetOsFromOSABI(header.e_ident[EI_OSABI], ostype);
 spec_ostype = arch_spec.GetTriple().getOS();
 assert(spec_ostype == ostype);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits