return a crc32 uuid when an ELF file has no build ID
#174
Conversation
andrurogerz
force-pushed
the
module-crc
branch
6 times, most recently
from
5198467 to
334e9cb
Compare
andrurogerz
force-pushed
the
module-crc
branch
from
334e9cb to
86c733f
Compare
andrurogerz
changed the title
uuid when an ELF file has no build IDuuid when an ELF file has no build ID
|
|
||
| template <typename ELFHeader, typename SectionHeader> | ||
| static bool ReadStringTable(int fd, const ELFHeader &ehdr, | ||
| SectionHeader &shdr, std::vector<char> &table); |
There was a problem hiding this comment.
I'm not sure how I feel about this. This is effectively a full copy of the string table. Do we need a copy? Is the mapped view not guaranteed to outlive this the reference? That is, more specifically, can we get away with a std::span<char> &?
There was a problem hiding this comment.
There is no mmap of the file at this point, just an open fd. This string table should just include section names so I don't expect it to ever be very large-- just one string per section.
Let me rename the method to ReadSectionNameStringTable for clarity.
| // return a crc32 of the contents of the file. The documentation for | ||
| // `qModuleInfo` suggests an md5 hash can be returned as "md5" in the | ||
| // response instead of "uuid." However, returning a crc32 is consistent | ||
| // with behavior of lldb-server. |
There was a problem hiding this comment.
Technically, MD5 is the right hash here. The default build ID hash was MD5, though it is not required to be MD5. The modern default has changed to SHA1, though UUID is becoming somewhat common too. The user also has the ability to specify a custom (hex) string of an even length as well. Is there some expectation that it is the CRC? I'd prefer MD5/SHA1 over CRC32.
There was a problem hiding this comment.
I would prefer to follow the docs and use md5 here, but there are a number of lldb tests that that start failing when we report an md5 hash instead of uuid. I don't know if sha1 is allowed-- there is nothing in the documentation.
This crc implementation is what I can observe lldb-server doing while running tests, and it gets the most tests passing. I will go back to md5 and see if there's anything more I can do to get tests passing.
| // response instead of "uuid." However, returning a crc32 is consistent | ||
| // with behavior of lldb-server. | ||
| uint32_t crc; | ||
| CHK(File::crc32(path, crc)); |
There was a problem hiding this comment.
Technically, the computation is over the normative portions of the file, not the entire file. I suppose that there is the performance aspect - digesting the entire file is faster than parsing and processing the normative parts. Not doing the slower process means that we would not be able to correctly identify files, which leaves this partially incorrect.
There was a problem hiding this comment.
From what I've observed, the CRCs calculated this way exactly match those sent by lldb-server on the same files. Am I getting lucky? Is there any documentation on what parts of the file should be covered by the crc/hash?
There was a problem hiding this comment.
Documentation? Ha! The source code to the linker is the documentation 😢. IIRC, it was .text, .data, and any custom sections that are digested.
| continue; | ||
|
|
||
| const std::string sectionName(&table[shdr.sh_name]); | ||
| if (sectionName != ".gnu_debuglink") |
There was a problem hiding this comment.
I wonder if we should create a global constant for the section name.
There was a problem hiding this comment.
I will. That was lazy.
|
I don't think making these changes will get us much, and the majority of the tests enabled in this PR were enabled without it in #177. |
2 participants
Purpose
In response to a
qModuleInfopacket, send a crc32 of the file contents as theuuidwhen an ELF file has no build ID. The change mimics the behavior observed inlldb-server, enabling a number of additional lldb test cases to pass against ds2 running on Linux and Android.Overview
.gnu_debuglinksection and return the crc32 value it contains as the "uuid".gnu_debuglinksection, calculate a crc32 checksum of the entire file contents and return it as the "uuid"File::crc32for Windows following the pattern of the otherFilemethods.TestGlobalModuleCache.GlobalModuleCacheTestCase.test_OneTargetOneDebuggertest cases on Linux because the test now fails (appears to be a bug in the test case)Validation
Ran the build & test GitHub workflow