[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 Jul 31 03:03:57 PDT 2018
jhenderson added a comment.
I'm not seeing anything that checks that the requested compression style is a supported style. I'd expect an error to be generated if a nonsensical version is specified.
On testing this. We now have no tests at all...? That's wrong.
================
Comment at: tools/llvm-objcopy/Object.cpp:495-496
+
+ if (!doCompressionFinalize)
+ return;
+
----------------
plotfi wrote:
> jhenderson wrote:
> > This bugs me. It maybe suggests to me that a compressed section should be a new section type.
> Can you elaborate? Not sure I understand what you mean (may have lost some context, sorry, most recent update I did -U9999).
This is similar to Jake's point about using OwnedDataSection. I don't want Section to need to be modifiable for all usages, so I think that a sub-class (maybe OwnedDataSection) should be used instead.
================
Comment at: tools/llvm-objcopy/Object.cpp:1311
+ bool doDecompress = DecompressDebugSections;
+ bool IsGnuStyle = (CompressDebugSections == DebugCompressionType::GNU);
----------------
Incorrect case here and below -> DoDecompress. But why not just use DecompressDebugSections directly?
================
Comment at: tools/llvm-objcopy/Object.cpp:1320
+ for (auto &Section : Obj.sections()) {
+
+ bool isCompressable = object::isCompressable(Section.Name);
----------------
Remove blank line here.
================
Comment at: tools/llvm-objcopy/Object.h:397
void finalize() override;
+ virtual bool isCompressable() const override {
+ StringRef GnuZDebugName =
----------------
plotfi wrote:
> plotfi wrote:
> > jhenderson wrote:
> > > alexshap wrote:
> > > > here and below - i think the implementations should be moved to Object.cpp
> > > Also, don't mark functions with both `virtual` and `override`. The `override` implies `virtual`, so only use the latter (similar to elsewhere in this file).
> > Yes.
> Thanks @jhenderson, I did not know that.
Just for added clarity:
`virtual` only has any semantic meaning on the highest version of the method in a class hierarchy. Overriding versions of methods in derived classes automatically inherit `virtual`. However, prior to C++11 it was common to explicitly mark these derived implementations with `virtual` themselves, for clarity. With C++11, `override` came into existence, and ensures that we are genuinely overriding a base-class virtual method, and as such it means we don't need the `virtual` tag for clarity any more.
================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:598
Config.KeepFileSymbols = InputArgs.hasArg(OBJCOPY_keep_file_symbols);
+ Config.DecompressDebugSections = InputArgs.hasArg(OBJCOPY_decompress_debug_sections);
for (auto Arg : InputArgs.filtered(OBJCOPY_localize_symbol))
----------------
clang-format
================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:612
+ Config.CompressDebugSections != DebugCompressionType::None) {
+ error("Can not specify --compress-debug-sections as well as "
+ "--decompress-debug-sections at the same time.");
----------------
Can not -> Cannot
Repository:
rL LLVM
https://reviews.llvm.org/D49678
More information about the llvm-commits
mailing list