This is an archive of the discontinued LLVM Phabricator instance.

[GSYM] Add GsymCreator and GsymReader.
ClosedPublic

Authored by clayborg on Oct 9 2019, 4:42 PM.

Details

Summary

This patch adds the ability to create GSYM files with GsymCreator, and read them with GsymReader. Full testing has been added for both new classes.

This patch differs from the original patch https://reviews.llvm.org/D53379 in that is uses a StringTableBuilder class from llvm instead of a custom version. Support for big and little endian files has been added. If the endianness matches the current host, we use efficient extraction for the header, address table and address info offset tables.

Diff Detail

Event Timeline

clayborg created this revision.Oct 9 2019, 4:42 PM

Mechanically, this looks mostly fine.

include/llvm/DebugInfo/GSYM/GsymCreator.h
140 ↗(On Diff #224204)

In other places we use uint8_t UUID[16] for this.

168 ↗(On Diff #224204)

Just personal opinion, but anything with locks I would put into the cpp file.

213 ↗(On Diff #224204)

same here

include/llvm/DebugInfo/GSYM/GsymReader.h
51 ↗(On Diff #224204)

///

68 ↗(On Diff #224204)

///

71 ↗(On Diff #224204)

why is this interface useful? Wouldn't an iterator of a ForEach function be cleaner?

lib/DebugInfo/GSYM/GsymCreator.cpp
1 ↗(On Diff #224204)

a -*- C++ -*- marker only makes sense in a .h file where the language is ambiguous.

196 ↗(On Diff #224204)

This is confusing to read because of all the nested-ness. Would it be possible to convert this into a

if (error) {
 OS << warning
 continue;
}

form?

lib/DebugInfo/GSYM/GsymReader.cpp
1 ↗(On Diff #224204)

same here

unittests/DebugInfo/GSYM/GSYMTest.cpp
1300 ↗(On Diff #224204)

clang-format

clayborg marked an inline comment as done.Oct 10 2019, 8:51 AM
clayborg added inline comments.
include/llvm/DebugInfo/GSYM/GsymCreator.h
140 ↗(On Diff #224204)

The users set the UUID with the setUUID function which takes an array ref and it currently copies all bytes. I could switch this to "uint8_t UUID[GSYM_MAX_UUID_SIZE];" and switch the error checking to be done on the call to setUUID. Right now the error handling happens when you try to encode.

clayborg updated this revision to Diff 224372.Oct 10 2019, 8:54 AM
  • Added full header documentation for GsymReader and GsymCreator.
  • Cleaned up and removed some function that were not used and seemed like they would be used for iteration
  • Made some GsymReader functions private and now with header doc intent of use should be clear
  • Renamed a few functions to better names.
clayborg marked 9 inline comments as done.Oct 10 2019, 8:57 AM
clayborg added inline comments.
lib/DebugInfo/GSYM/GsymCreator.cpp
197 ↗(On Diff #224372)

Not really because of the trickyness of erasing values form the Funcs. Note the "Curr = Funcs.erase(Prev);" statements in here. It is also why the while loop containing this can't be a simple for loop. I tried playing with this a bit and it stayed complicated, so left as is for now. Let me know if you still have objections.

aprantl accepted this revision.Oct 10 2019, 9:51 AM
This revision is now accepted and ready to land.Oct 10 2019, 9:51 AM

$ svn commit
Sending include/llvm/DebugInfo/GSYM/FileWriter.h
Adding include/llvm/DebugInfo/GSYM/GsymCreator.h
Adding include/llvm/DebugInfo/GSYM/GsymReader.h
Sending include/llvm/DebugInfo/GSYM/Header.h
Sending lib/DebugInfo/GSYM/CMakeLists.txt
Sending lib/DebugInfo/GSYM/FunctionInfo.cpp
Adding lib/DebugInfo/GSYM/GsymCreator.cpp
Adding lib/DebugInfo/GSYM/GsymReader.cpp
Sending lib/DebugInfo/GSYM/Header.cpp
Sending unittests/DebugInfo/GSYM/CMakeLists.txt
Sending unittests/DebugInfo/GSYM/GSYMTest.cpp
Transmitting file data ...........done
Committing transaction...
Committed revision 374381.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2019, 10:13 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
thakis added a subscriber: thakis.Oct 10 2019, 10:48 AM

This doesn't build on Windows: http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/11397

C:\b\slave\clang-x64-windows-msvc\build\llvm.src\lib\DebugInfo\GSYM\GsymReader.cpp(17): fatal error C1083: Cannot open include file: 'sys/mman.h': No such file or directory

The fix attempt didn't work:

ECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_UNICODE -DUNICODE -I../../llvm/include -Igen/llvm/include /O2 /Zc:inline /EHs-c- /W4 -Wno-unused-parameter -Wdelete-non-virtual-dtor -Wstring-conversion -Wno-nonportable-include-path -Wcovered-switch-default /GR-
../../llvm/lib/DebugInfo/GSYM/GsymCreator.cpp(76,3): error: use of undeclared identifier 'bzero'
  bzero(Hdr.UUID, sizeof(Hdr.UUID));
  ^
1 error generated.
[16/106] CXX obj/llvm/lib/DebugInfo/GSYM/GSYM.GsymReader.obj
FAILED: obj/llvm/lib/DebugInfo/GSYM/GSYM.GsymReader.obj 
c:\src\goma\goma-win64/gomacc c:/src/chrome/src/third_party/llvm-build/Release+Asserts/bin/clang-cl /nologo /showIncludes /Foobj/llvm/lib/DebugInfo/GSYM/GSYM.GsymReader.obj /c ../../llvm/lib/DebugInfo/GSYM/GsymReader.cpp -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_UNICODE -DUNICODE -I../../llvm/include -Igen/llvm/include /O2 /Zc:inline /EHs-c- /W4 -Wno-unused-parameter -Wdelete-non-virtual-dtor -Wstring-conversion -Wno-nonportable-include-path -Wcovered-switch-default /GR-
../../llvm/lib/DebugInfo/GSYM/GsymReader.cpp(19,10): fatal error: 'unistd.h' file not found
#include <unistd.h>

The fix attempt didn't work:

ECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_UNICODE -DUNICODE -I../../llvm/include -Igen/llvm/include /O2 /Zc:inline /EHs-c- /W4 -Wno-unused-parameter -Wdelete-non-virtual-dtor -Wstring-conversion -Wno-nonportable-include-path -Wcovered-switch-default /GR-
../../llvm/lib/DebugInfo/GSYM/GsymCreator.cpp(76,3): error: use of undeclared identifier 'bzero'
  bzero(Hdr.UUID, sizeof(Hdr.UUID));
  ^
1 error generated.
[16/106] CXX obj/llvm/lib/DebugInfo/GSYM/GSYM.GsymReader.obj
FAILED: obj/llvm/lib/DebugInfo/GSYM/GSYM.GsymReader.obj 
c:\src\goma\goma-win64/gomacc c:/src/chrome/src/third_party/llvm-build/Release+Asserts/bin/clang-cl /nologo /showIncludes /Foobj/llvm/lib/DebugInfo/GSYM/GSYM.GsymReader.obj /c ../../llvm/lib/DebugInfo/GSYM/GsymReader.cpp -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_UNICODE -DUNICODE -I../../llvm/include -Igen/llvm/include /O2 /Zc:inline /EHs-c- /W4 -Wno-unused-parameter -Wdelete-non-virtual-dtor -Wstring-conversion -Wno-nonportable-include-path -Wcovered-switch-default /GR-
../../llvm/lib/DebugInfo/GSYM/GsymReader.cpp(19,10): fatal error: 'unistd.h' file not found
#include <unistd.h>

$ svn commit
Sending lib/DebugInfo/GSYM/GsymCreator.cpp
Transmitting file data .done
Committing transaction...
Committed revision 374409.
$ svn commit
Sending lib/DebugInfo/GSYM/GsymReader.cpp
Transmitting file data .done
Committing transaction...
Committed revision 374410.

Switched to memset and removed include of unistd.h

That's still not enough, you need to remove a bunch of (unused) includes. With this, my windows box is happy. (I don't have svn setup on my win box, so I can't land this easily):

diff --git a/llvm/lib/DebugInfo/GSYM/GsymCreator.cpp b/llvm/lib/DebugInfo/GSYM/GsymCreator.cpp
index 9dc632dfcfb..f371426f201 100644
--- a/llvm/lib/DebugInfo/GSYM/GsymCreator.cpp
+++ b/llvm/lib/DebugInfo/GSYM/GsymCreator.cpp
@@ -14,7 +14,8 @@

 #include <algorithm>
 #include <cassert>
-#include <strings.h>
+#include <functional>
+#include <vector>

 using namespace llvm;
 using namespace gsym;
@@ -73,7 +74,7 @@ llvm::Error GsymCreator::encode(FileWriter &O) const {
   Hdr.NumAddresses = static_cast<uint32_t>(Funcs.size());
   Hdr.StrtabOffset = 0; // We will fix this up later.
   Hdr.StrtabOffset = 0; // We will fix this up later.
-  bzero(Hdr.UUID, sizeof(Hdr.UUID));
+  memset(Hdr.UUID, 0, sizeof(Hdr.UUID));
   if (UUID.size() > sizeof(Hdr.UUID))
     return createStringError(std::errc::invalid_argument,
                              "invalid UUID size %u", (uint32_t)UUID.size());
diff --git a/llvm/lib/DebugInfo/GSYM/GsymReader.cpp b/llvm/lib/DebugInfo/GSYM/GsymReader.cpp
index f7bbd700713..dfb585b87c1 100644
--- a/llvm/lib/DebugInfo/GSYM/GsymReader.cpp
+++ b/llvm/lib/DebugInfo/GSYM/GsymReader.cpp
@@ -10,17 +10,8 @@
 #include "llvm/DebugInfo/GSYM/GsymReader.h"

 #include <assert.h>
-#include <fcntl.h>
 #include <inttypes.h>
-#include <stdio.h>
 #include <stdlib.h>
-#include <sys/stat.h>
-#include <sys/types.h>
-#include <unistd.h>
-
-#include <fstream>
-#include <functional>
-#include <vector>

 #include "llvm/DebugInfo/GSYM/GsymCreator.h"
 #include "llvm/DebugInfo/GSYM/InlineInfo.h"

I found a computer with commit set up, so fixed in r374413.

I found a computer with commit set up, so fixed in r374413.

Thank you!!!