Re: [PATCH v8 0/11 Add support for CRC64 generation in linker

[Nick Clifton <nickc@redhat.com>]

Hi Ulf,

   The patch set is looking good so far.

   I have been running some experiments on misusing the DIGEST directive
   and found a few problems.  For example, if I want to have two digests
   in my binary:

      % ld foo.o -T test.t
     ld:test.t:19: warning: Only the first CRC polynome is used
     ld:crc.t:0: warning: updating CRC failed
     CRC generation aborted

   which would be fine, except for the fact that the link succeeds:

     % echo $?
     0

  (Hint: You can use %X in your message string when calling einfo() to set
   an error exit code...)

   Even better would be to allow an number of digests.  I still do not know
   why you want to have a limit of 1.


   I do not like using the DIGEST SECTION directive to specify the section
   for the digest.  This should be implied by the presence of the DIGEST
   directive itself.  For example:

     .data : { DIGEST ... }

   ought to create a digest in the .data section.  Furthermore it seems that
   the DIGEST SECTION directive does not work reliably:

     % cat test.t
     _start = 0x000000;
     SECTIONS
     {
       . = 0x1000 + SIZEOF_HEADERS;

       .foo ALIGN (CONSTANT (COMMONPAGESIZE)) :
        {
           foo_start = .;
           *(.foo)
           foo_end = .;
         }

         .data ALIGN (CONSTANT (COMMONPAGESIZE)) :
         {
           DIGEST SECTION ".foo"
           DIGEST "BE.BE.BE" POLY (64,a,b,c,d,e,f) (foo_start , foo_end)
           *(.data)
         }

        /DISCARD/ : { *(*) }
     }

     % ld -T test.t foo.o
     ld:test.t:0: warning: updating CRC failed
     CRC generation aborted


   Another issue - if the digest is unrecognised then the error message
   is not very helpful.  For example I tried:

      DIGEST "0.BE" "TCRC32" (___CRC32_START___ , text_end)

   and received the error message:

     ld: Unknown DIGEST: no error

   (There is no line number in the error message, nor does it repeat of the
   unrecognized name).

   Incidentally if a valid digest name is used in the above example a symbol
   with the name "0" is generated, which is not really allowed as symbol
   names should not start with a numeric value.  But that is probably being
   too pedantic.


   Oh - and here is a good one.  If the start symbol is beyond the end symbol
   the code triggers a segmentation fault:

     % cat test.t
     [...]
     .text ALIGN (CONSTANT (COMMONPAGESIZE)) :
       {
         DIGEST "BE.BE.BE" POLY (64,a,b,c,d,e,f) (text_end , text_start)
         text_start = .;
         *(.text)
         text_end = .;
       }
      [...]

     % ld foo.o -T test.t
     Segmentation fault (core dumped)


   I am not sure if the next one matters or not, but just in case: The
   checksum and crc table can be generated on unaligned boundaries if
   the code does not happen to be aligned.  For example:

     .text ALIGN (CONSTANT (COMMONPAGESIZE)) :
       {
         BYTE(0);
         DIGEST "BE.BE.BE" POLY (64,a,b,c,d,e,f) (text_start , text_end)
         text_start = .;
         *(.text)
         text_end = .;
       }


   If the new directives are used outside of an output section then they
   do not generate very helpful error messages.  For example:

     % cat test.t
     _start = 0x000000;
     SECTIONS
     {
       DEBUG ON
       /DISCARD/ : { *(*) }
     }

     % ld foo.o -T test.t
     ld:test.t:5: syntax error

   (Note the incorrect line number for the error).  This is probably a limitation
   of the grammar supported by bison however, so I would not be surprised if there
   is no easy way to fix it.


   On a related note - I think that there is a problem with the checksum
   generation.  Specifically using the ".BE" suffix to indicate that a
   big-endian checksum should be computed.  The problem is that some targets
   support both big-endian and little-endian output, selectable via a
   command line option, and so a static choice in a linker script is
   inappropriate.  May I suggest an alternative ?  Support both ".LE" and
   ".BE" suffixes, and if neither is specified then use whichever endian
   format has been selected by the command line.

Cheers
   Nick