This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [gold][PATCH] PR gold/18321: gold doesn't support SHF_COMPRESSED sections


> This patch adds SHF_COMPRESSED section decompression to gold.  OK for
> master?

+         bool is_compressed
+           = (shdr.get_sh_flags() & elfcpp::SHF_COMPRESSED) != 0;

Please match the style of surrounding code. In gold, when we break
a line at '=', we break after the '=' and indent 4 spaces.

-             uint64_t uncompressed_size = get_uncompressed_size(contents, len);
+               obj->section_contents(i, &len, false);

See above.

+               compression_header_size
+                 = elfcpp::Elf_sizes<size>::chdr_size;

This will fit on one line.

+             uint64_t uncompressed_size
+               = get_uncompressed_size(contents + compression_header_size,
+                                       len - compression_header_size);

In new-style compressed sections, the uncompressed size is in the
compression header. See my comments below.

+             info.flag = convert_to_section_size_type(shdr.get_sh_flags());

You shouldn't be using convert_to_section_size_type on sh_flags.

              if (uncompressed_size != -1ULL)
                {
+                 if (is_compressed)
+                   {
+                     typename elfcpp::Chdr<size, big_endian> chdr(contents);
+                     if (chdr.get_ch_type() != elfcpp::ELFCOMPRESS_ZLIB
+                         || chdr.get_ch_size() != uncompressed_size
+                         || (chdr.get_ch_addralign()
+                             != shdr.get_sh_addralign()))
+                       return uncompressed_map;
+                   }
                  unsigned char* uncompressed_data = NULL;
-                 if (decompress_if_needed && need_decompressed_section(name))
+                 if (decompress_if_needed
+                     && (is_compressed
+                         || need_decompressed_section(name)))
                    {
                      uncompressed_data = new unsigned char[uncompressed_size];
-                     if (decompress_input_section(contents, len,
+                     if (decompress_input_section(contents +
compression_header_size,
+                                                  len -
compression_header_size,
                                                   uncompressed_data,
                                                   uncompressed_size))
                        info.contents = uncompressed_data;

I don't see how this is going to work right. This patch doesn't do anything
to decompress_input_section, so in an SHF_COMPRESS section, it looks like
you're expecting both an Elf compression header *and* the old-style "ZLIB"
plus 8 big-endian bytes. That's wrong. In a new-style compressed section,
the raw compressed data should begin immediately after the compression
header. For reference from the gABI proposal:

 ELFCOMPRESS_ZLIB
        The section data is compressed with the ZLIB compression algorithm.
        The compressed ZLIB data bytes begin with the byte immediately
        following the compression header, and extend to the end of the
        section. Additional documentation for ZLIB may be found at
        http://zlib.net/.

I'd add a flag to get_uncompressed_size and decompress_input_section, let
them deal with the difference between the two styles, and greatly simplify
the logic in this function.

If the test case is passing, that makes me suspect that the assembler is
also wrong.

+  if (memmem(names, sd->section_names_size, ".zdebug_", 8) != NULL
+      || memmem(names, sd->section_names_size, ".debug_", 7) != NULL)

Scanning the section strings table for ".zdebug_" is a kludge, necessary
only because we didn't have a section type or flag to scan for.  Rather
than extend this kludge to the ABI-compliant implementation, I'd rather
scan the section headers for the SHF_COMPRESSED flag. If we find any,
there's no need to look for ".zdebug_".

+  if ((p->second.flag & elfcpp::SHF_COMPRESSED) != 0)
+    {
+      const int size = parameters->target().get_size();
+      if (size == 32)
+       compression_header_size = elfcpp::Elf_sizes<32>::chdr_size;
+      else if (size == 64)
+       compression_header_size = elfcpp::Elf_sizes<64>::chdr_size;
+      else
+       gold_unreachable();
+    }
+  else
+    compression_header_size = 0;

This logic should be in decompress_input_section.

+  if (!decompress_input_section(buffer + compression_header_size,
+                               buffer_size - compression_header_size,
                                uncompressed_data,
                                uncompressed_size))

As above, decompress_input_section needs to deal with either an old-style
compression header, or a new-style one -- not a combination of the two.

 struct Compressed_section_info
 {
   section_size_type size;
+  section_size_type flag;

Why is this section_size_type? It should be elfcpp::Elf_Xword.

+         if ((shdr.get_sh_flags() & elfcpp::SHF_COMPRESSED) != 0)
+            compression_header_size = elfcpp::Elf_sizes<size>::chdr_size;
+         else
+           compression_header_size = 0;
+         if (!decompress_input_section(p + compression_header_size,
+                                       len - compression_header_size,
+                                       view, view_size))

See above.

-cary


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]