[PATCH] D46628: [ELF] Add --strip-debug-non-line option
Lucian Adrian Grijincu via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jun 24 06:13:27 PDT 2019
luciang added a comment.
Sorry, I forgot I didn't submit comments.
================
Comment at: lld/ELF/Driver.cpp:1853
+ .EndsWith("debug_str", false)
+ .StartsWith(".debug", true)
+ .StartsWith(".zdebug", true)
----------------
MaskRay wrote:
> ```
> if (S->Name.consume_front(".debug_") || S->Name.consume_front(".zdebug_")) {
> ...
> }
> ```
Can't use `consume_front`: that changes the `Name` of the section.
```
/// Returns true if this StringRef has the given prefix and removes that
/// prefix.
bool consume_front(StringRef Prefix) {
if (!startswith(Prefix))
return false;
*this = drop_front(Prefix.size());
return true;
}
```
================
Comment at: lld/ELF/OutputSections.cpp:205
+ ReducedDebugData.clear();
+
----------------
MaskRay wrote:
> Neither `vector::clear` nor `SmallVector::clear` deallocates the storage. Did you intend to call `shrink_to_fit()`?
I used `clear` here not to free memory, but to signal that `ReducedDebugData` doesn't hold information anymore -- it has been moved to `CompressedData`.
I did call `shrink_to_fit` as you suggested in the std::string version, but later switched to `SmallString` and swapped with a temporary to free memory.
================
Comment at: lld/ELF/OutputSections.h:116
+ // Reduced .debug_info/.debug_abbrev when using --strip-debug-non-line.
+ llvm::SmallVector<char, 1> ReducedDebugData;
+
----------------
luciang wrote:
> MaskRay wrote:
> > Is size 1 `ReducedDebugData` common? If not, a container other than `SmallVector<char, 1>` may be better.
> I used the same container as for CompressedData bellow. I'll use std::vector. Anything is fine here as I only allocate memory once.
I can't use a std::vector as I mentioned above. I couldn't find a raw_ostream implementation that prints to a std::vector.
I only found https://llvm.org/doxygen/classllvm_1_1raw__string__ostream.html
- raw_svector_ostream which prints to a SmallVector
- raw_string_ostream which prints to a std::string
I'll go with a std::string here.
================
Comment at: lld/ELF/Writer.cpp:1073
+template <typename value_type>
+void writeInt(raw_ostream &OS, value_type value, endianness endian) {
+ value = byte_swap<value_type>(value, endian);
----------------
MaskRay wrote:
> Delete this helper.
>
> Use `endian::write(OS, UINT32_C(0xffffffff), Endian);` below.
>
> (this utility is defined in llvm/Support/EndianStream.h)
Unfortunately I can't include that header as it defines another `Writer` class which clashes with the one defined in this file:
This is what happens if I include it:
```
$ git diff
diff --git a/lld/ELF/Writer.cpp b/lld/ELF/Writer.cpp
index 6f16cf21daf..b3c54b47251 100644
--- a/lld/ELF/Writer.cpp
+++ b/lld/ELF/Writer.cpp
@@ -28,6 +28,7 @@
#include "llvm/BinaryFormat/Dwarf.h"
#include "llvm/DebugInfo/DWARF/DWARFFormValue.h"
#include "llvm/Support/DataExtractor.h"
+#include "llvm/Support/EndianStream.h"
#include "llvm/Support/LEB128.h"
#include "llvm/Support/RandomNumberGenerator.h"
#include "llvm/Support/SHA1.h"
```
```
/home/lucian/local/github/llvm-project/lld/ELF/Writer.cpp: In function ‘void lld::elf::writeResult()’:
/home/lucian/local/github/llvm-project/lld/ELF/Writer.cpp:152:49: error: reference to ‘Writer’ is ambiguous
template <class ELFT> void elf::writeResult() { Writer<ELFT>().run(); }
^
/home/lucian/local/github/llvm-project/lld/ELF/Writer.cpp:51:29: note: candidates are: template<class ELFT> class {anonymous}::Writer
template <class ELFT> class Writer {
^
In file included from /home/lucian/local/github/llvm-project/lld/ELF/Writer.cpp:31:0:
/home/lucian/local/github/llvm-project/llvm/include/llvm/Support/EndianStream.h:51:8: note: struct llvm::support::endian::Writer
struct Writer {
^
/home/lucian/local/github/llvm-project/lld/ELF/Writer.cpp:152:60: error: expected primary-expression before ‘>’ token
template <class ELFT> void elf::writeResult() { Writer<ELFT>().run(); }
^
/home/lucian/local/github/llvm-project/lld/ELF/Writer.cpp:152:62: error: expected primary-expression before ‘)’ token
template <class ELFT> void elf::writeResult() { Writer<ELFT>().run(); }
^
```
I could re-name the Writer class in this file, but wanted to keep the diff focused on the task.
================
Comment at: lld/ELF/Writer.cpp:1161
+ std::unordered_map<uint32_t, uint32_t> InfoOffMap;
+ SmallVector<char, 1> ReducedInfo;
+ // Reduce .debug_info
----------------
MaskRay wrote:
> SmallString<0>
>
> The allocation is likely unavoidable. Don't waste stack space.
I had all of these as `SmallString<1, char>` earlier and switched to `std::string` after a previous comment. I misunderstood your intent there. I switched all of them to `SmallString<0>`.
================
Comment at: lld/ELF/Writer.cpp:1181
+
+ uint32_t NextInfoOffset = InfoOffset + Length;
+ uint16_t Version = InfoData.getU16(&InfoOffset);
----------------
MaskRay wrote:
> `NextInfoOffset` is only used once. Just `InfoOffset += Length` below.
`InfoOffset` is updated by the `getU8` & co. calls.
http://dwarfstd.org/doc/DWARF5.pdf
7.5.1.1 Full and Partial Compilation Unit Headers
```
unit_length (initial length)
A 4-byte or 12-byte unsigned integer representing the length of the
.debug_info contribution for that compilation unit, not including the length
field itself. In the 32-bit DWARF format, this is a 4-byte unsigned integer
(which must be less than 0xfffffff0); in the 64-bit DWARF format, this
consists of the 4-byte value 0xffffffff followed by an 8-byte unsigned
integer that gives the actual length (see Section 7.4 on page 196).
```
`Length` is the length of this CU + all its children. I don't copy the children into the reduced buffer, just the DIE for the CU.
At the end of the loop bellow `InfoOffset` bellow will only point to the end of the DIE corresponding to this CU. I need it to skip the children too.
As written here `NextInfoOffset` will point to the next CU. I'll keep this behavior.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D46628/new/
https://reviews.llvm.org/D46628
More information about the llvm-commits
mailing list