[PATCH] D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections.
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 7 05:59:55 PDT 2018
jhenderson added inline comments.
================
Comment at: tools/llvm-objcopy/Object.h:269-272
+ virtual const StringRef getContents() const {
+ assert(false && "Section has no Contents.");
+ return "";
+ }
----------------
plotfi wrote:
> jhenderson wrote:
> > Don't mark return types as const (except for pointers to const and reference returns): the caller can just ignore the const and copy the value, so it has zero use (indeed some compilers actually warn about this). It's also completely redundant for a StringRef, since StringRefs are implicitly const.
> >
> > Should this function just be a pure abstract function if it can't be called?
> >
> > I also don't know whether "getContents" means get original contents or get contents at the end. Please rename more appropriately.
> What do you mean? I guess the tricky thing here is, it depends on the type of object. I don't see this being useful for anything other than a Section or a OwnedDataSection or a Compressed/Decompressed section.
Right, so should this be a part of the base section interface? What you are saying suggests that it shouldn't be.
Is the purpose of this function to get the original, unmodified section contents, or the contents after they get modified (when the latter is possible)?
================
Comment at: tools/llvm-objcopy/Object.h:368
+
+ CompressedSection(std::unique_ptr<SmallVectorImpl<char>> CompressedData)
+ : ModifiedData(std::move(CompressedData)) {}
----------------
plotfi wrote:
> jhenderson wrote:
> > I'm pretty sure you're supposed to use `SmallVector` directly.
> But a SmallVector of what size? As far as I know SmallVector always has some size N. In a lot of places when I see SmallVector passed around (as in the case of raw_ostream) it's passed as a SmallVectorImpl&. Should this not be the case here?
Okay, my bad. I just feel like a class with "impl" in the name implies it's intended for internal use only...!
But actually, it strikes me that this isn't "Small" anyway, so should probably just be a std::vector directly (that suggests that the compress function interface is incorrect then).
================
Comment at: tools/llvm-objcopy/Object.h:353
+
+class CompressedSection : public SectionBase {
+ MAKE_SEC_WRITER_FRIEND
----------------
This should inherit from OwnedDataSection, so that it doesn't need to reimplement data ownership.
================
Comment at: tools/llvm-objcopy/Object.h:372
+ object::getDebugSectionName(Sec.Name, IsGnuStyle);
+ assert(NewName && "Expected a valid debug section name.");
+ setName(IsGnuStyle ? *NewName : Sec.Name.str());
----------------
Don't use asserts to check an Expected. This will cause an abort in a build without asserts. If a library function can return an Error (or Expected), we need to handle them correctly (usually report an error).
================
Comment at: tools/llvm-objcopy/Object.h:384
+
+class DecompressedSection : public SectionBase {
+ MAKE_SEC_WRITER_FRIEND
----------------
It looks to me like DecompressedSection could actually be just OwnedDataSection. I don't think we need a separate class for it. You would just need to change OwnedDataSection to own its own name.
================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:354
+std::unique_ptr<SmallVectorImpl<char>>
+decompress(StringRef &Name, StringRef Contents, ElfType OutputElfType) {
+ switch (OutputElfType) {
----------------
jakehehrlich wrote:
> I think you don't need to do this if you create a 'DecompressedSectionType' that knows how to handle write out the decompressed contents because the writer will be able to call the proper decompress<ELFT> method directly!
@plotfi, you've marked this comment as "Done", yet I don't see you using ELFT directly anywhere. You should be able to use ELFT::TargetEndianness and ELFT::Is64Bits in the library call (although personally, I think that the library interface should take an ELFT, so that these don't need explicitly passing in).
================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:333
+decompress(StringRef Name, StringRef Contents, ElfType OutputElfType) {
+ auto setElfType = [](ElfType OutputElfType) -> std::tuple<bool, bool> {
+ switch (OutputElfType) {
----------------
What are you setting here? There is no change in state in this function.
================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:364
+
+object::CompressedResult compress(const StringRef Contents, uint64_t Align,
+ bool IsGnuStyle, ElfType OutputElfType) {
----------------
Don't bother marking Contents as const. StringRefs are equivalent to const & std::string already.
================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:379
+
+static void tryCompressSections(const CopyConfig &Config, Object &Obj,
+ SectionPred &RemovePred,
----------------
I think you can just call this "compressSections". You don't need the "try" part.
================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:383-386
+ if (object::isCompressed(Section.Name, Section.Flags))
+ continue;
+ if (!object::isCompressable(Section.Name))
+ continue;
----------------
Combine these two ifs into a single one, please.
================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:388
+
+ auto ContentsWrapped = Section.getSectionContents();
+ if (!ContentsWrapped) {
----------------
Don't use auto here. Rename the variable to ContentsOrErr or similar, in keeping with typical naming of Expected results.
================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:391
+ reportError(Config.InputFilename, ContentsWrapped.takeError());
+ continue;
+ }
----------------
No need to `continue`. `reportError` exits the program.
================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:393-394
+ }
+ StringRef Contents((const char *)ContentsWrapped->data(),
+ ContentsWrapped->size());
+ auto Result =
----------------
Don't turn this into a StringRef here. Leave it as an ArrayRef. The compression functions should probably operate on ArrayRefs not StringRefs (including in the existing library) in my opinion, but even if we can't change that decision, we should delay the conversion until necessary.
Also, don't use C-style casts.
================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:397
+ compress(Contents, Section.Align,
+ (Config.CompressDebugSections == DebugCompressionType::GNU),
+ OutputElfType);
----------------
Rather than use Config.CompressDebugSections directly here, pass it in as a function parameter, i.e. the signature of this function should look something like the following:
```tryCompressSections(const CopyConfig &Config, Object &Obj,
DebugCompressionType CompressionType, SectionPred &RemovePred,
ElfType OutputElfType)
```
Then propagate `CompressionType` into `compress` and do the comparison there.
================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:401
+ bool IsSmaller = std::get<1>(Result);
+ bool DoBail = !IsSmaller || (nullptr == std::get<2>(Result));
+ Error Err = std::move(std::get<0>(Result));
----------------
I don't know what the second Result parameter is - please assign it to a separate variable.
================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:404
+ if (Err) {
+ DoBail = true;
+ reportError(Config.InputFilename, std::move(Err));
----------------
No need to set DoBail here, since `reportError` exits the program.
================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:406-408
+ } else {
+ consumeError(std::move(Err));
+ }
----------------
Remove this else clause. `consumeError` has no purpose here.
================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:410
+
+ if (DoBail)
+ continue;
----------------
I don't think you need `DoBail` I think you can do the check inline, since it's only needed once.
================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:421
+
+static void tryDecompressSections(const CopyConfig &Config, Object &Obj,
+ SectionPred &RemovePred,
----------------
Many of my comments for `tryCompressSections` also apply to this function.
================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:829
+ InputArgs.getLastArgValue(OBJCOPY_compress_debug_sections) != "") {
+ error("Invaid or unsupported --compress-debug-sections format: " +
+ InputArgs.getLastArgValue(OBJCOPY_compress_debug_sections));
----------------
Invaid -> Invalid.
Please add a test for this error.
================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:886
+ Config.CompressDebugSections != DebugCompressionType::None) {
+ error("Cannot specify --compress-debug-sections as well as "
+ "--decompress-debug-sections at the same time.");
----------------
Please add a test for this error too.
Repository:
rL LLVM
https://reviews.llvm.org/D49678
More information about the llvm-commits
mailing list