This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] - Fix incorrect CompressedSection creation.
ClosedPublic

Authored by grimar on Mar 6 2019, 3:05 AM.

Details

Summary

We should create CompressedSection only if the section has SHF_COMPRESSED flag
or it's name starts from '.zdebug'. Currently, we create it if data starts from ZLIB signature.
As a result, if you call isa<CompressedSection> on a section created, it would return false,
because classof checks the section name and not the ZLIB signature (what is correct):
https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objcopy/ELF/Object.h#L384

There is no test, as because of broken isa described such sections are handled as uncompressed:
https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objcopy/ELF/Object.cpp#L237

So patch does not change the behavior observed but allows to create the correct sections
from start.

Diff Detail

Event Timeline

grimar created this revision.Mar 6 2019, 3:05 AM
grimar edited the summary of this revision. (Show Details)Mar 6 2019, 3:06 AM
grimar retitled this revision from [llvm-objdump] - Fix incorrect CompressedSection creation. to [llvm-objcopy] - Fix incorrect CompressedSection creation..Mar 6 2019, 3:32 AM

The code change looks good, but I'm not sure I understand why you can't test this, i.e. where you have an input with a compressed section starting with .zdebug and showing that it is decompressed? I've forgotten the design a bit though, so perhaps you could clarify when you get CompressedSection instances and what they are used for?

grimar added a comment.EditedMar 6 2019, 4:55 AM

The code change looks good, but I'm not sure I understand why you can't test this, i.e. where you have an input with a compressed section starting with .zdebug and showing that it is decompressed?

The case when we have a compressed section starting with .zdebug is tested in compress-debug-sections-zlib-gnu.test I think.

Here we have a bit different problem. Imagine you have a section .foo (so its name does not start from .z*). And if that .foo has ZLIB bytes at the start,
then the current code will create CompressedSection. But it is incorrect because .foo is not a gnu style compressed section because of its name.

if we have no compression flags, that will just work like if .foo would be a regular section.
(https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objcopy/ELF/Object.cpp#L237)

If we have --decompress-debug-sections flag then the following code will be called:

replaceDebugSections(
    Config, Obj, RemovePred,
    [](const SectionBase &S) { return isa<CompressedSection>(&S); },
    [&Obj](const SectionBase *S) {
      auto CS = cast<CompressedSection>(S);
      return &Obj.addSection<DecompressedSection>(*CS);
    });

But since isa<CompressedSection>(&S); will return false, this section will not be converted to DecompressedSection
and we will handle it as a regular section.

So I can't test this, code already just works, though it is definitely not correct to create .foo as a compressed section.

jhenderson accepted this revision.Mar 6 2019, 5:28 AM

Okay, LGTM.

This revision is now accepted and ready to land.Mar 6 2019, 5:28 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2019, 6:02 AM