public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
From: Jan Beulich <jbeulich@suse.com>
To: Binutils <binutils@sourceware.org>
Cc: Dave Korn <dave.korn.cygwin@gmail.com>
Subject: [PATCH 1/5] don't over-align file positions of PE executable sections
Date: Fri, 6 May 2022 08:54:02 +0200	[thread overview]
Message-ID: <d9432c9f-fa0e-0cba-2bd5-13f3149c4df7@suse.com> (raw)
In-Reply-To: <4242b48a-f2c3-4af2-db1e-35dbbbdc1b2e@suse.com>

When a sufficiently small alignment was specified via --file-alignment,
individual section alignment shouldn't affect placement within the file.
This involves first of all clearing D_PAGED for images when section and
file alignment together don't permit paging of the image. The involved
comparison against COFF_PAGE_SIZE in turn helped point out (through a
compiler warning) that 'page_size' should be of unsigned type (as in
particular FileAlignment is). This yet in turn pointed out a dubious
error condition (which is being deleted).

For the D_PAGED case I think the enforced file alignment may still be
too high, but I'm wary of changing that logic without knowing of
possible corner cases.

Furthermore file positions in PE should be independent of the alignment
recorded in section headers anyway. Otherwise there are e.g. anomalies
following commit 6f8f6017a0c4 ("PR27567, Linking PE files adds alignment
section flags to executables") in that linking would use information a
subsequent processing step (e.g. stripping) wouldn't have available
anymore, and hence a binary could change in that 2nd step for no actual
reason. (Similarly stripping a binary linked with a linker pre-dating
that commit would change the binary again when stripping it a 2nd time.)
---
The new condition for clearing D_PAGED may be a little too strict now;
the alternative would be to check all section RVAs and file positions
to match modulo COFF_PAGE_SIZE. One could think that it should be the
other way around (file positions _only_ be assigned to satisfy that
condition), but then there would be no way to request sections to be
arranged in this more space saving manner. (Note that if that
condition was adjusted, other changes made later in the function then
would also need further refinement.)

Quite likely the referenced commit should be follow by another change
suppressing also the reading of the section alignment bits when
processing a PE binary. This would similarly eliminate the oddity of
stripping a previously stripped binary altering that binary again.

--- a/bfd/coffcode.h
+++ b/bfd/coffcode.h
@@ -2962,7 +2962,7 @@ coff_compute_section_file_positions (bfd
 #endif
 
 #ifdef COFF_IMAGE_WITH_PE
-  int page_size;
+  unsigned int page_size;
 
   if (coff_data (abfd)->link_info
       || (pe_data (abfd) && pe_data (abfd)->pe_opthdr.FileAlignment))
@@ -2973,22 +2973,12 @@ coff_compute_section_file_positions (bfd
 	 This repairs 'ld -r' for arm-wince-pe target.  */
       if (page_size == 0)
 	page_size = 1;
-
-      /* PR 17512: file: 0ac816d3.  */
-      if (page_size < 0)
-	{
-	  bfd_set_error (bfd_error_file_too_big);
-	  _bfd_error_handler
-	    /* xgettext:c-format */
-	    (_("%pB: page size is too large (0x%x)"), abfd, page_size);
-	  return false;
-	}
     }
   else
     page_size = PE_DEF_FILE_ALIGNMENT;
 #else
 #ifdef COFF_PAGE_SIZE
-  int page_size = COFF_PAGE_SIZE;
+  unsigned int page_size = COFF_PAGE_SIZE;
 #endif
 #endif
 
@@ -3070,9 +3060,10 @@ coff_compute_section_file_positions (bfd
     bfd_size_type amt;
 
 #ifdef COFF_PAGE_SIZE
-    /* Clear D_PAGED if section alignment is smaller than
-       COFF_PAGE_SIZE.  */
-   if (pe_data (abfd)->pe_opthdr.SectionAlignment < COFF_PAGE_SIZE)
+    /* Clear D_PAGED if section / file alignment aren't suitable for
+       paging at COFF_PAGE_SIZE granularity.  */
+   if (pe_data (abfd)->pe_opthdr.SectionAlignment < COFF_PAGE_SIZE
+       || page_size < COFF_PAGE_SIZE)
      abfd->flags &= ~D_PAGED;
 #endif
 
@@ -3193,7 +3184,11 @@ coff_compute_section_file_positions (bfd
 	     padding the previous section up if necessary.  */
 	  old_sofar = sofar;
 
+#ifdef COFF_IMAGE_WITH_PE
+	  sofar = BFD_ALIGN (sofar, page_size);
+#else
 	  sofar = BFD_ALIGN (sofar, 1 << current->alignment_power);
+#endif
 
 #ifdef RS6000COFF_C
 	  /* Make sure the file offset and the vma of .text/.data are at the
@@ -3269,7 +3264,11 @@ coff_compute_section_file_positions (bfd
       else
 	{
 	  old_sofar = sofar;
+#ifdef COFF_IMAGE_WITH_PE
+	  sofar = BFD_ALIGN (sofar, page_size);
+#else
 	  sofar = BFD_ALIGN (sofar, 1 << current->alignment_power);
+#endif
 	  align_adjust = sofar != old_sofar;
 	  current->size += sofar - old_sofar;
 	}


  reply	other threads:[~2022-05-06  6:54 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-06  6:53 [PATCH 0/5] PE/COFF: assorted adjustments Jan Beulich
2022-05-06  6:54 ` Jan Beulich [this message]
2022-05-06  6:54 ` [PATCH 2/5] COFF: make objcopy / strip honor --keep-file-symbols Jan Beulich
2022-05-06  6:54 ` [PATCH 3/5] COFF/PE: don't leave zero timestamp after objcopy / strip Jan Beulich
2022-05-06  6:55 ` [PATCH 4/5] COFF/PE: keep linker version during " Jan Beulich
2022-05-06  6:55 ` [PATCH 5/5] COFF: use hash for string table also when copying / stripping Jan Beulich

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d9432c9f-fa0e-0cba-2bd5-13f3149c4df7@suse.com \
    --to=jbeulich@suse.com \
    --cc=binutils@sourceware.org \
    --cc=dave.korn.cygwin@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).