RFC: Add initial support for .NET Core dlls to objdump

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

RFC: Add initial support for .NET Core dlls to objdump

Omair Majid
Hi,

Recent versions of .NET Core ship with some dll (PE/COFF) files that
can't be parsed by objdump:

    $ objdump -x /usr/lib64/dotnet/shared/Microsoft.NETCore.App/2.1.11/System.dll
    objdump: /usr/lib64/dotnet/shared/Microsoft.NETCore.App/2.1.11/System.dll: file format not recognized

It seems like these files have a slightly different value for the
IMAGE_FILE_HEADER.Machine field than normal dlls. In particular, the "normal"
architecture-based magic value is XOR'ed with an OS-specific value to get the
final magic value. [1]

Allowing the new magic values lets objdump get started:

    $ ~/local/binutils/bin/objdump -x dotnet/shared/Microsoft.NETCore.App/2.1.11/System.Runtime.dll
    dotnet/shared/Microsoft.NETCore.App/2.1.11/System.Runtime.dll:   file format pei-x86-64
    dotnet/shared/Microsoft.NETCore.App/2.1.11/System.Runtime.dll
    architecture: i386:x86-64, flags 0x0000012f:
    HAS_RELOC, EXEC_P, HAS_LINENO, HAS_DEBUG, HAS_LOCALS, D_PAGED

    Characteristics 0x2022
        executable
        large address aware
        DLL

    Time/Date               Wed Jun  5 14:49:41 2019
    Magic                   020b    (PE32+)
    ...


Some open questions:

0. Should this "non-stanard" magic field in the dll be exposed somewhere
   in the objdump UI?

1. Should I add tests for these? If so, any pointers on how to do that?

2. I added the new flags for architecture/OS combination for the binaries I
   could find. Should I try and find out what the magic flags for other
   architecture/OS combinations (bsds? arm64?) are? Even if I don't have
   access to binary dlls that demonstrate this?

3. Since this touches shared code, do I need to have this patch reviewed
   elsewhere too?

This is my first patch for binutils, so I would appreciate it someone can tell
me about any other mistakes I am making (or about to make) :)

Thanks,
Omair

[1] https://github.com/jbevain/cecil/issues/337
--
PGP Key: B157A9F0 (http://pgp.mit.edu/)
Fingerprint = 9DB5 2F0B FD3E C239 E108  E7BD DF99 7AF8 B157 A9F0

0001-Handle-some-pe-files-generated-by-.NET.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Add initial support for .NET Core dlls to objdump

Nick Clifton
Hi Omair,

> 0. Should this "non-stanard" magic field in the dll be exposed somewhere
>    in the objdump UI?

Yes.  Probably in the output for -p/--private-headers would be best.  Have
a look at the _bfd_XX_print_private_bfd_data_common function in bfd/peXXigen.c.


> 1. Should I add tests for these? If so, any pointers on how to do that?

If it is easy to do then yes.  But I suspect that it might be difficult
to construct a file containing the new machine field value.  If you can
do that however then the best place to add one or more tests would be the
binutils/testsuite/binutils-all directory.  Possibly as an addition to the
objdump.exp file, or else as a new stand alone test.


> 2. I added the new flags for architecture/OS combination for the binaries I
>    could find. Should I try and find out what the magic flags for other
>    architecture/OS combinations (bsds? arm64?) are? Even if I don't have
>    access to binary dlls that demonstrate this?

Yes please.  In cases like this it is best to try to cover as many variants
as possible, even if you cannot test them all.  If there are any errors then
at some point in the future someone will file a bug report pointing out the
problem.  But if you get the values right first time then everyone will be
happy, and no one will have to be nailed to a tree for doing it.  (Sorry -
slipped into misquoting the Hitch Hikers' Guide to the Galaxy there).


> 3. Since this touches shared code, do I need to have this patch reviewed
>    elsewhere too?

No - the file you are touching are all considered to be part of the binutils
project, even if they are shared with others.


> This is my first patch for binutils, so I would appreciate it someone can tell
> me about any other mistakes I am making (or about to make) :)

One thing missing from the patch, which is probably not your fault at all,
is comment containing a URL to the official documentation for these new
values.  I am of course assuming that there is some official documentation,
and that it is publicly accessible...

A couple of very minor points on the patch itself:

> --- a/bfd/ChangeLog
> +++ b/bfd/ChangeLog

In the future please could you provide the changelog entries just as simple
text, rather than as context diffs ?  The problem is that the changelogs
change so frequently that a context diff will often fail to apply.


> +/* Used in .NET DLLs that target non-Windows platforms */

Comments should be formatted as sentences.  So they should end with a period
and be followed by two spaces before the closing-comment characters.

But basically the patch - so far - looks great.

Cheers
  Nick
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Add initial support for .NET Core dlls to objdump

Omair Majid
Hi Nick,

Thanks for reviewing the patch and the great feedback!

* Nick Clifton <[hidden email]> [2019-07-01 08:11]:
> > 0. Should this "non-stanard" magic field in the dll be exposed somewhere
> >    in the objdump UI?
>
> Yes.  Probably in the output for -p/--private-headers would be best.  Have
> a look at the _bfd_XX_print_private_bfd_data_common function in bfd/peXXigen.c.

I am running into a bit of a wall here. I need to access the original
value of internal_filehdr->f_magic here. But that's not accessible at
this point.

I took a look at dump_bfd_header in objdump.c and that works with
processed bdf entries too.

Any pointers on how I can get a hold of the original f_magic field?

> > 1. Should I add tests for these? If so, any pointers on how to do that?
>
> If it is easy to do then yes.  But I suspect that it might be difficult
> to construct a file containing the new machine field value.  If you can
> do that however then the best place to add one or more tests would be the
> binutils/testsuite/binutils-all directory.  Possibly as an addition to the
> objdump.exp file, or else as a new stand alone test.

Done. At least two dll files are now generated "by hand" when tests are
run.

> > 2. I added the new flags for architecture/OS combination for the binaries I
> >    could find. Should I try and find out what the magic flags for other
> >    architecture/OS combinations (bsds? arm64?) are? Even if I don't have
> >    access to binary dlls that demonstrate this?
>
> Yes please.  In cases like this it is best to try to cover as many variants
> as possible, even if you cannot test them all.  If there are any errors then
> at some point in the future someone will file a bug report pointing out the
> problem.  But if you get the values right first time then everyone will be
> happy, and no one will have to be nailed to a tree for doing it.  (Sorry -
> slipped into misquoting the Hitch Hikers' Guide to the Galaxy there).
If it's okay with you, can I do that as a separate patch? I would like
to take care of take care of aarch64 (and others platforms and
architectures) separately instead of making this patch much larger.

> One thing missing from the patch, which is probably not your fault at all,
> is comment containing a URL to the official documentation for these new
> values.  I am of course assuming that there is some official documentation,
> and that it is publicly accessible...

I added some links to some sources I have been looking at, but even
ECMA-335 says that only one valid value is possible, which we know is
no longer true. The best and the most concise source I can find is bug
report for a project:
https://github.com/jbevain/cecil/issues/337

The .NET Core sources are available under the MIT license:
https://github.com/dotnet/coreclr/blob/6f7aa7967c607b8c667518314ab937c0d7547025/src/inc/pedecoder.h#L94-L107

Is that something I can use/link to as "a source of
truth/documentation"?

I have attached an updated patch.

bfd/ChangeLog:

2019-07-03  Omair Majid  <[hidden email]>

        * coffcode.h (coff_set_arch_mach_hook): Handle
        I386_NATIVE_LINUX_MAGIC, I386_NATIVE_APPLE_MAGIC,
        AMD64_NATIVE_LINUX_MAGIC and AMD64_NATIVE_LINUX_MAGIC.
        * peXXigen.c: Add references to ECMA-335.

binutils/ChangeLog:

2019-07-03  Omair Majid  <[hidden email]>

        * binutils/Makefile.amh (GENTESTDLLS_PROG): Define.
        (TEST_PROGS): Add GENTESTDLLS_PROG.
        (gentestdlls_DEPENDENCIES): Define.
        * gentestdlls.c: New file.
        * testsuite/binutils-all/objdump.exp
        (test_objdump_dotnet_assemblies): Define.

include/ChangeLog:

2019-07-03  Omair Majid <[hidden email]>

       * coff/pe.h (IMAGE_FILE_MACHINE_NATIVE_LINUX_OVERRIDE),
       (IMAGE_FILE_MACHINE_NATIVE_APPLE_OVERRIDE): Define.
       * coff/i386.h (I386_NATIVE_LINUX_MAGIC),
       (I386_NATIVE_APPLE_MAGIC): Define.
       (I386BADMAG): Extend to include the above.
       * coff/x86_64.h (AMD64_NATIVE_LINUX_MAGIC),
       (AMD64_NATIVE_APPLE_MAGIC): Define.
       (AMD64BADMAG): Extend to include the above.

Omair

--
PGP Key: B157A9F0 (http://pgp.mit.edu/)
Fingerprint = 9DB5 2F0B FD3E C239 E108  E7BD DF99 7AF8 B157 A9F0

0001-Handle-some-PE-COFF-files-generated-by-.NET.patch (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Add initial support for .NET Core dlls to objdump

Nick Clifton
Hi Omair,

>>> 0. Should this "non-stanard" magic field in the dll be exposed somewhere
>>>    in the objdump UI?
>>
>> Yes.  Probably in the output for -p/--private-headers would be best.  Have
>> a look at the _bfd_XX_print_private_bfd_data_common function in bfd/peXXigen.c.
>
> I am running into a bit of a wall here. I need to access the original
> value of internal_filehdr->f_magic here. But that's not accessible at
> this point.
>
> I took a look at dump_bfd_header in objdump.c and that works with
> processed bdf entries too.
>
> Any pointers on how I can get a hold of the original f_magic field?

In theory you could call coff_set_flags() and that you give you the
f_magic value as a return value.  But in practice this will give you
the f_magic value that the BFD library *thinks* that the output file
should have, and not the f_magic value that was read in.  

Incidentally this raises another issue - it looks like the BFD library
will not generate PE files with these new magic values, so if you run
say "objcopy a.exe b.exe" then b.exe will not have the same f_magic as
a.exe.  Assuming that a.exe had the new f_magic value.

Anyway, so the short answer to your question is that you cannot access
the f_magic value of the input bfd.  The long answer is that the way to
solve this is to create new bfd architecture structures to handle the
new f_magic values.  Then you can have coff_set_arch_mach_hook() return
the new architectures and coff_set_flags() return new f_magic values for
those architectures.  If you would like to have a go at doing this then
please do...
 
>>> 2. I added the new flags for architecture/OS combination for the binaries I
>>>    could find. Should I try and find out what the magic flags for other
>>>    architecture/OS combinations (bsds? arm64?) are?

> If it's okay with you, can I do that as a separate patch?

Sure - that is not a problem.


> The .NET Core sources are available under the MIT license:
> https://github.com/dotnet/coreclr/blob/6f7aa7967c607b8c667518314ab937c0d7547025/src/inc/pedecoder.h#L94-L107
>
> Is that something I can use/link to as "a source of
> truth/documentation"?

Sure, it does not hurt.

> I have attached an updated patch.

Thanks - there are a few issues with the patch that I will mention here:

> +static int write_dos_header_and_stub(FILE* file);
> +static int write_pe_signature(FILE* file);
> +static int write_coff_header(FILE* file, uint16_t machine);

You do not need prototypes for static functions unless they are
used before they are defined in the source file.  (And generally
speaking it is better to define them first if possible, as this
does help some compilers produce better code).

Also - I note that all three of these functions return an integer
value which is always 0, and which is always ignored.  It would
be better to just have them be void functions...

Also please include a single white space between the end of the
function name and the opening parenthesis of its arguments.  eg:

  write_dos_header_and_stub (FILE* file)

This applies both when declaring the function and when using it.


> +  /*
> +   * See ECMA-335 II.25.2.1.
> +   *
> +   * Instead of lfanew, lets just hardcode the offset of the next byte
> +   * after this header (0x80).
> +   */

Multiline comments should not have line prefixes, should be treated as a
normal paragraph and should end on the last line of the comment.  ie:

   /* See ECMA-335 II.25.2.1.
      Instead of lfanew, lets just hardcode the offset of the next byte
      after this header (0x80).  */


> +  char buffer[128] = {

Curly braces should be moved to the line below the declaration, rather than
ending a line.  ie:

    char buffer[128] =
    {

I also think that since the gentestdlls.c program is part of the binutils
testsuite it ought to live in the binutils/testsuite directory and not the
binutils/ directory.  Also you need a mechanism to build the gentestdlls
executable, and to bail out from the test if the executable cannot be created.

Thanks for persisting with this patch.

Cheers
  Nick

PS.  I am off on PTO for two weeks, so I will not be able to respond to emails
until later this month...
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Add initial support for .NET Core dlls to objdump

Omair Majid

Hi Nick,

Thanks for all your comments! They have been really helpful.

Nick Clifton <[hidden email]> writes:

> I also think that since the gentestdlls.c program is part of the binutils
> testsuite it ought to live in the binutils/testsuite directory and not the
> binutils/ directory.

I see that some other programs - bfdtest1 and bfdtest2 - also work
similarly. I have left them in binutils instead of moving them into
binutils/testsuite.

> Also you need a mechanism to build the gentestdlls
> executable, and to bail out from the test if the executable cannot be created.

The changes to Makefile.am take care of this already, no? I did run
autoreconf locally in both binutils-gdb and in binutils-gdb/binutils. I
did not include the changes in my patch because it causes a large diff,
and I also seem to have a different version of automake (1.16.1 vs 1.15.1).

As I understand it, if gentestdlls fails to build, it fails the entire
binutils build. Is that okay?

Do I have to handle other architectures in the test specially? Will
objdump be able to handle dumping i386 PE/Coff on aarch64 platforms, for
example?

> PS.  I am off on PTO for two weeks, so I will not be able to respond to emails
> until later this month...

Hope you are enjoying your time off!

Cheers,
Omair

--
PGP Key: B157A9F0 (http://pgp.mit.edu/)
Fingerprint = 9DB5 2F0B FD3E C239 E108  E7BD DF99 7AF8 B157 A9F0

0001-Handle-some-PE-COFF-files-generated-by-.NET.patch (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Add initial support for .NET Core dlls to objdump

Nick Clifton
Hi Omair,

  Thanks for the revised patch.  I have now checked it in, although
  I did have to make a couple of changes:  The binutils test now copes
  with targets which do not recognise the PE file format, and the
  additions to the header files will now work on targets which only
  support 32-bit PE format files.

Cheers
  Nick

PS.  I also created some changelog entries for you:

include/ChangeLog
2019-07-23  Omar Majid  <[hidden email]>

        * coff/i386.h (IMAGE_FILE_MACHINE_NATIVE_APPLE_OVERRIDE): Define.
        (IMAGE_FILE_MACHINE_NATIVE_FREEBSD_OVERRIDE): Define.
        (IMAGE_FILE_MACHINE_NATIVE_LINUX_OVERRIDE): Define.
        (IMAGE_FILE_MACHINE_NATIVE_NETBSD_OVERRIDE): Define.
        (I386_APPLE_MAGIC): Define.
        (I386_FREEBSD_MAGIC): Define.
        (I386_LINUX_MAGIC): Define.
        (I386_NETBSD_MAGIC): Define.
        (I386BADMAG): Extend macro to allow new magic numbers.
        * coff/x86_64.h (IMAGE_FILE_MACHINE_NATIVE_APPLE_OVERRIDE): Define.
        (IMAGE_FILE_MACHINE_NATIVE_FREEBSD_OVERRIDE): Define.
        (IMAGE_FILE_MACHINE_NATIVE_LINUX_OVERRIDE): Define.
        (IMAGE_FILE_MACHINE_NATIVE_NETBSD_OVERRIDE): Define.
        (AMD64_APPLE_MAGIC): Define.
        (AMD64_FREEBSD_MAGIC): Define.
        (AMD64_LINUX_MAGIC): Define.
        (AMD64_NETBSD_MAGIC): Define.
        (AMD64BADMAG): Extend macro to allow new magic numbers.

bfd/Changelog
2019-07-23  Omar Majid  <[hidden email]>

        * coffcode.h (coff_set_arch_mach_hook): Handle I386_APPLE_MAGIC,
        I386_FREEBSD_MAGIC, I386_LINUX_MAGIC, I386_NETBSD_MAGIC,
        AMD64_APPLE_MAGIC, AMD64_FREEBSD_MAGIC, AMD64_LINUX_MAGIC,
        AMD64_NETBSD_MAGIC.
        * peXXigen.c: Add comment about source of .NET magic numbers.

binutils/ChangeLog
2019-07-23  Omar Majid  <[hidden email]>

        * Makefile.am (AUTOMAKE_OPTIONS): Add subdir-objects
        (GENTESTDLLSPROG): Define.
        (TEST_PROGS): Add GENTESTDLLSPROG.
        * Makefile.in: Regenerate.
        * testsuite/binutils-all/objdump.exp
        (test_objdump_dotnet_assemblies): New proc.
        Run the new proc.
        * testsuite/gentestdlls.c: New source file.
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Add initial support for .NET Core dlls to objdump

Omair Majid

Hi Nick,

Nick Clifton <[hidden email]> writes:

>   Thanks for the revised patch.  I have now checked it in

Thanks for checking it in! I was expecting a few more rounds of patches,
so this was a *very* nice surprise!

>   although
>   I did have to make a couple of changes:  The binutils test now copes
>   with targets which do not recognise the PE file format

This might be a stupid question, but how does the test deal with
regressions in dll parsing? It seems to me like the test would pass even
if objudmp can't parse System.Runtime.dll. The "file format not
recognized" is the same error that a objdump built without the fix
prints out:

$ objdump -x /usr/lib64/dotnet/shared/Microsoft.NETCore.App/2.1.12/System.Runtime.dll
objdump: /usr/lib64/dotnet/shared/Microsoft.NETCore.App/2.1.12/System.Runtime.dll: file format not recognized

>   and the
>   additions to the header files will now work on targets which only
>   support 32-bit PE format files.

Ah, thank you. Is there a way I can discover this myself (for next
time)? Would ./configure --enable-targets=all flag this for me?

> PS.  I also created some changelog entries for you:

Apologies. I was expecting a few more rounds of patches so I figured I
could be a bit sloppy and skip this. Sorry.

Regards,
Omair

--
PGP Key: B157A9F0 (http://pgp.mit.edu/)
Fingerprint = 9DB5 2F0B FD3E C239 E108  E7BD DF99 7AF8 B157 A9F0
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Add initial support for .NET Core dlls to objdump

Nick Clifton
Hi Omair,

> This might be a stupid question,

Actually it is not stupid at all...

> but how does the test deal with
> regressions in dll parsing? It seems to me like the test would pass even
> if objudmp can't parse System.Runtime.dll. The "file format not
> recognized" is the same error that a objdump built without the fix
> prints out:
>
> $ objdump -x /usr/lib64/dotnet/shared/Microsoft.NETCore.App/2.1.12/System.Runtime.dll
> objdump: /usr/lib64/dotnet/shared/Microsoft.NETCore.App/2.1.12/System.Runtime.dll: file format not recognized

Well bananas.  I had not considered that.

I think that what we need to do is to extend gentestdlls.c so that
it generates a normal x86 PE dll file as well.  Then the test can
be extended to check this file first.  If the target objdump is
able to parse this file, then it should be able to parse the other
two files as well.  If it cannot then there is an error.  (Actually
32-bit PE toolchains may only be able to parse one of the new .NET
PE dlls).


>>   additions to the header files will now work on targets which only
>>   support 32-bit PE format files.
>
> Ah, thank you. Is there a way I can discover this myself (for next
> time)? Would ./configure --enable-targets=all flag this for me?

Nope.  You need a target that includes the 32-bit PE header but not
the 64-bit PE header.  I used "./configure --target=i686-pc-cygwin".

Cheers
  Nick
Reply | Threaded
Open this post in threaded view
|

RFC: Make dotnet assemblies test more robust

Omair Majid

Hi Nick,

Nick Clifton <[hidden email]> writes:

> I think that what we need to do is to extend gentestdlls.c so that
> it generates a normal x86 PE dll file as well.  Then the test can
> be extended to check this file first.  If the target objdump is
> able to parse this file, then it should be able to parse the other
> two files as well.  If it cannot then there is an error.  (Actually
> 32-bit PE toolchains may only be able to parse one of the new .NET
> PE dlls).

Thanks for the idea! How does the attached patch look?

ChangeLog:

2019-07-25  Omair Majid  <[hidden email]>

        * testsuite/binutils-all/objdump.exp
        (test_objdump_dotnet_assemblies): Fix test to distinguish errors
        in parsing simple pei-i386 and pei-x86-64 vs parsing the newly
        introduced machine types.
        * testsuite/gentestdlls.c (write_simple_dll): New function.
        (main): Generate simple and Linux-specific variants of pei-i386
        and pei-x86-64 files so both can be used by tests.
Thanks,
Omair

--
PGP Key: B157A9F0 (http://pgp.mit.edu/)
Fingerprint = 9DB5 2F0B FD3E C239 E108  E7BD DF99 7AF8 B157 A9F0


From 7cd938670308d429e20e1f72aa72a87c499de892 Mon Sep 17 00:00:00 2001
From: Omair Majid <[hidden email]>
Date: Thu, 25 Jul 2019 18:31:53 -0400
Subject: [PATCH] Make objdump PEI tests a little more robust/portable

---
 binutils/testsuite/binutils-all/objdump.exp | 25 ++++--
 binutils/testsuite/gentestdlls.c            | 91 +++++++++++++--------
 2 files changed, 74 insertions(+), 42 deletions(-)

diff --git a/binutils/testsuite/binutils-all/objdump.exp b/binutils/testsuite/binutils-all/objdump.exp
index b46fd0a8bb..1b65486479 100644
--- a/binutils/testsuite/binutils-all/objdump.exp
+++ b/binutils/testsuite/binutils-all/objdump.exp
@@ -740,15 +740,16 @@ proc test_objdump_dotnet_assemblies {} {
 
     set test "dotnet-assemblies"
 
-    set got [binutils_run "$base_dir/testsuite/gentestdlls" "tmpdir"]
-    set want "wrote dotnet-linux-x86-64.dll"
+    set got [binutils_run "$base_dir/testsuite/gentestdlls" "tmpdir pei-i386 pei-x86-64"]
+    set want "wrote linux-pei-x86-64.dll"
+    # The test program is hardcoded to generate valid dlls on any target
     if ![regexp $want $got] then {
- unsupported "$test"
+        fail "$test"
     }
 
     set test "dotnet-assemblies (32-bit)"
     set want "file format pei-i386"
-    set got [binutils_run $OBJDUMP "-x tmpdir/simple-i386.dll"]
+    set got [binutils_run $OBJDUMP "-x tmpdir/simple-pei-i386.dll"]
     if ![regexp $want $got] then {
  if [regexp "file format not recognized" $got] then {
     unsupported $test
@@ -756,12 +757,17 @@ proc test_objdump_dotnet_assemblies {} {
     fail "$test"
  }
     } else {
- pass $test
+        set got [binutils_run $OBJDUMP "-x tmpdir/linux-pei-i386.dll"]
+        if ![regexp $want $got] then {
+            fail "$test"
+        } else {
+            pass $test
+        }
     }
 
     set test "dotnet-assemblies (64-bit)"
     set want "file format pei-x86-64"
-    set got [binutils_run $OBJDUMP "-x tmpdir/dotnet-linux-x86-64.dll"]
+    set got [binutils_run $OBJDUMP "-x tmpdir/simple-pei-x86-64.dll"]
     if ![regexp $want $got] then {
  if [regexp "file format not recognized" $got] then {
     unsupported $test
@@ -769,7 +775,12 @@ proc test_objdump_dotnet_assemblies {} {
     fail "$test"
  }
     } else {
- pass $test
+        set got [binutils_run $OBJDUMP "-x tmpdir/linux-pei-x86-64.dll"]
+        if ![regexp $want $got] then {
+            fail "$test"
+        } else {
+            pass $test
+        }
     }
 }
 
diff --git a/binutils/testsuite/gentestdlls.c b/binutils/testsuite/gentestdlls.c
index b1463c0178..f52806814f 100644
--- a/binutils/testsuite/gentestdlls.c
+++ b/binutils/testsuite/gentestdlls.c
@@ -29,8 +29,12 @@
 #include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <string.h>
 #include <unistd.h>
 
+#define INCORRECT_USAGE 2
+#define IO_ERROR 3
+
 static void
 write_dos_header_and_stub (FILE* file)
 {
@@ -100,53 +104,70 @@ write_coff_header (FILE* file, uint16_t machine)
   memset (buffer, 0 , sizeof (buffer));
 }
 
-int
-main (int argc, char** argv)
+static void
+write_simple_dll(const char* name, uint16_t machine)
 {
-  FILE* file;
-
-  if (argc < 2)
-    {
-      fprintf (stderr, "usage: %s output-directory\n", argv[0]);
-      exit (2);
-    }
-  if (chdir (argv[1]) != 0)
-    {
-      fprintf (stderr, "error: unable to change directory to %s\n", argv[0]);
-      exit (2);
-    }
-
-  /* Generate a simple DLL file.  */
-  file = fopen ("simple-i386.dll", "w");
+  FILE* file = fopen (name, "w");
   if (file == NULL)
     {
       fprintf (stderr, "error: unable to open file for writing\n");
-      exit (1);
+      exit (IO_ERROR);
     }
 
   write_dos_header_and_stub (file);
   write_pe_signature (file);
-  write_coff_header (file, 0x14c);
+  write_coff_header (file, machine);
   fclose (file);
-  printf ("wrote simple-i386.dll\n");
-
-  /* Generate a sample .NET Core on Linux dll file.  As opposed to the
-     more common DLLs that contain bytecode (CIL/MSIL), many .NET Core
-     DLLs are pre-compiled for specific architectures and platforms.
-     See https://github.com/jbevain/cecil/issues/337 for an example of
-     this value being used in practice.  */
-  file = fopen ("dotnet-linux-x86-64.dll", "w");
-  if (file == NULL)
+  file = NULL;
+  printf ("wrote %s\n", name);
+}
+
+int
+main (int argc, char** argv)
+{
+  char* program_name = argv[0];
+  if (argc < 3)
     {
-      fprintf (stderr, "error: unable to open file for writing\n");
-      exit (1);
+      fprintf (stderr, "usage: %s output-directory format [format ...] \n\n", program_name);
+      fprintf (stderr, "format is an objdump-style format string, like pei-i386\n");
+      exit (INCORRECT_USAGE);
+    }
+  char* output_directory = argv[1];
+  if (chdir (output_directory) != 0)
+    {
+      fprintf (stderr, "error: unable to change directory to %s\n", output_directory);
+      exit (INCORRECT_USAGE);
     }
 
-  write_dos_header_and_stub (file);
-  write_pe_signature (file);
-  write_coff_header (file, 0xfd1d /* x86-64 + Linux */);
-  fclose (file);
-  printf ("wrote dotnet-linux-x86-64.dll\n");
+  /* We generate a simple PEI format files, and then .NET Core on
+     Linux-style PEI files for a number of architectures.  As opposed
+     to the more common PEI files that contain bytecode (CIL/MSIL), many
+     .NET Core DLLs are pre-compiled for specific architectures and
+     platforms.  See https://github.com/jbevain/cecil/issues/337 for an
+     example of this value being used in practice.  */
+
+  for (int i = 2; i < argc; i++)
+    {
+      char* wanted_format = argv[i];
+
+      if (strcmp ("pei-i386", wanted_format))
+        {
+          write_simple_dll ("simple-pei-i386.dll", 0x14c);
+
+          write_simple_dll ("linux-pei-i386.dll", 0x14c ^ 0x7b79 /* i386 + Linux */);
+        }
+      else if (strcmp ("pei-x86-64", wanted_format))
+        {
+          write_simple_dll ("simple-pei-x86-64.dll", 0x8664);
+
+          write_simple_dll ("linux-pei-x86-64.dll", 0x8664 ^ 0x7b79 /* x86-64 + Linux */);
+        }
+      else
+        {
+          fprintf (stderr, "error: can't handle format %s\n", wanted_format);
+          exit (INCORRECT_USAGE);
+        }
+    }
 
   return 0;
 }
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

Re: RFC: Make dotnet assemblies test more robust

Omair Majid

Omair Majid <[hidden email]> writes:

> Nick Clifton <[hidden email]> writes:
>
>> I think that what we need to do is to extend gentestdlls.c so that
>> it generates a normal x86 PE dll file as well.  Then the test can
>> be extended to check this file first.  If the target objdump is
>> able to parse this file, then it should be able to parse the other
>> two files as well.  If it cannot then there is an error.  (Actually
>> 32-bit PE toolchains may only be able to parse one of the new .NET
>> PE dlls).
>
> Thanks for the idea! How does the attached patch look?

Oops. I just realized that I made a mistake in strcmp. Fixed now.

binutils/ChangeLog:

2019-07-25  Omair Majid  <[hidden email]>

        * testsuite/binutils-all/objdump.exp
        (test_objdump_dotnet_assemblies): Fix test to distinguish errors
        in parsing simple pei-i386 and pei-x86-64 vs parsing the newly
        introduced machine types.
        * testsuite/gentestdlls.c (write_simple_dll): New function.
        (main): Generate simple and Linux-specific variants of pei-i386
        and pei-x86-64 files so both can be used by tests.
Omair

--
PGP Key: B157A9F0 (http://pgp.mit.edu/)
Fingerprint = 9DB5 2F0B FD3E C239 E108  E7BD DF99 7AF8 B157 A9F0


===File
/home/omajid/devel/binutils-gdb/0001-Make-objdump-PEI-tests-a-little-more-robust-portable.patch===
From 1070dacd022500d2dcb72b7d60f13921600d3b6b Mon Sep 17 00:00:00 2001
From: Omair Majid <[hidden email]>
Date: Thu, 25 Jul 2019 18:31:53 -0400
Subject: [PATCH] Make objdump PEI tests a little more robust/portable

---
 binutils/testsuite/binutils-all/objdump.exp | 25 ++++--
 binutils/testsuite/gentestdlls.c            | 91 +++++++++++++--------
 2 files changed, 74 insertions(+), 42 deletions(-)

diff --git a/binutils/testsuite/binutils-all/objdump.exp b/binutils/testsuite/binutils-all/objdump.exp
index b46fd0a8bb..1b65486479 100644
--- a/binutils/testsuite/binutils-all/objdump.exp
+++ b/binutils/testsuite/binutils-all/objdump.exp
@@ -740,15 +740,16 @@ proc test_objdump_dotnet_assemblies {} {

     set test "dotnet-assemblies"

-    set got [binutils_run "$base_dir/testsuite/gentestdlls" "tmpdir"]
-    set want "wrote dotnet-linux-x86-64.dll"
+    set got [binutils_run "$base_dir/testsuite/gentestdlls" "tmpdir pei-i386 pei-x86-64"]
+    set want "wrote linux-pei-x86-64.dll"
+    # The test program is hardcoded to generate valid dlls on any target
     if ![regexp $want $got] then {
- unsupported "$test"
+        fail "$test"
     }

     set test "dotnet-assemblies (32-bit)"
     set want "file format pei-i386"
-    set got [binutils_run $OBJDUMP "-x tmpdir/simple-i386.dll"]
+    set got [binutils_run $OBJDUMP "-x tmpdir/simple-pei-i386.dll"]
     if ![regexp $want $got] then {
  if [regexp "file format not recognized" $got] then {
     unsupported $test
@@ -756,12 +757,17 @@ proc test_objdump_dotnet_assemblies {} {
     fail "$test"
  }
     } else {
- pass $test
+        set got [binutils_run $OBJDUMP "-x tmpdir/linux-pei-i386.dll"]
+        if ![regexp $want $got] then {
+            fail "$test"
+        } else {
+            pass $test
+        }
     }

     set test "dotnet-assemblies (64-bit)"
     set want "file format pei-x86-64"
-    set got [binutils_run $OBJDUMP "-x tmpdir/dotnet-linux-x86-64.dll"]
+    set got [binutils_run $OBJDUMP "-x tmpdir/simple-pei-x86-64.dll"]
     if ![regexp $want $got] then {
  if [regexp "file format not recognized" $got] then {
     unsupported $test
@@ -769,7 +775,12 @@ proc test_objdump_dotnet_assemblies {} {
     fail "$test"
  }
     } else {
- pass $test
+        set got [binutils_run $OBJDUMP "-x tmpdir/linux-pei-x86-64.dll"]
+        if ![regexp $want $got] then {
+            fail "$test"
+        } else {
+            pass $test
+        }
     }
 }

diff --git a/binutils/testsuite/gentestdlls.c b/binutils/testsuite/gentestdlls.c
index b1463c0178..bebe831e92 100644
--- a/binutils/testsuite/gentestdlls.c
+++ b/binutils/testsuite/gentestdlls.c
@@ -29,8 +29,12 @@
 #include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <string.h>
 #include <unistd.h>

+#define INCORRECT_USAGE 2
+#define IO_ERROR 3
+
 static void
 write_dos_header_and_stub (FILE* file)
 {
@@ -100,53 +104,70 @@ write_coff_header (FILE* file, uint16_t machine)
   memset (buffer, 0 , sizeof (buffer));
 }

-int
-main (int argc, char** argv)
+static void
+write_simple_dll(const char* name, uint16_t machine)
 {
-  FILE* file;
-
-  if (argc < 2)
-    {
-      fprintf (stderr, "usage: %s output-directory\n", argv[0]);
-      exit (2);
-    }
-  if (chdir (argv[1]) != 0)
-    {
-      fprintf (stderr, "error: unable to change directory to %s\n", argv[0]);
-      exit (2);
-    }
-
-  /* Generate a simple DLL file.  */
-  file = fopen ("simple-i386.dll", "w");
+  FILE* file = fopen (name, "w");
   if (file == NULL)
     {
       fprintf (stderr, "error: unable to open file for writing\n");
-      exit (1);
+      exit (IO_ERROR);
     }

   write_dos_header_and_stub (file);
   write_pe_signature (file);
-  write_coff_header (file, 0x14c);
+  write_coff_header (file, machine);
   fclose (file);
-  printf ("wrote simple-i386.dll\n");
-
-  /* Generate a sample .NET Core on Linux dll file.  As opposed to the
-     more common DLLs that contain bytecode (CIL/MSIL), many .NET Core
-     DLLs are pre-compiled for specific architectures and platforms.
-     See https://github.com/jbevain/cecil/issues/337 for an example of
-     this value being used in practice.  */
-  file = fopen ("dotnet-linux-x86-64.dll", "w");
-  if (file == NULL)
+  file = NULL;
+  printf ("wrote %s\n", name);
+}
+
+int
+main (int argc, char** argv)
+{
+  char* program_name = argv[0];
+  if (argc < 3)
     {
-      fprintf (stderr, "error: unable to open file for writing\n");
-      exit (1);
+      fprintf (stderr, "usage: %s output-directory format [format ...] \n\n", program_name);
+      fprintf (stderr, "format is an objdump-style format string, like pei-i386\n");
+      exit (INCORRECT_USAGE);
+    }
+  char* output_directory = argv[1];
+  if (chdir (output_directory) != 0)
+    {
+      fprintf (stderr, "error: unable to change directory to %s\n", output_directory);
+      exit (INCORRECT_USAGE);
     }

-  write_dos_header_and_stub (file);
-  write_pe_signature (file);
-  write_coff_header (file, 0xfd1d /* x86-64 + Linux */);
-  fclose (file);
-  printf ("wrote dotnet-linux-x86-64.dll\n");
+  /* We generate a simple PEI format files, and then .NET Core on
+     Linux-style PEI files for a number of architectures.  As opposed
+     to the more common PEI files that contain bytecode (CIL/MSIL), many
+     .NET Core DLLs are pre-compiled for specific architectures and
+     platforms.  See https://github.com/jbevain/cecil/issues/337 for an
+     example of this value being used in practice.  */
+
+  for (int i = 2; i < argc; i++)
+    {
+      char* wanted_format = argv[i];
+
+      if (strcmp ("pei-i386", wanted_format) == 0)
+        {
+          write_simple_dll ("simple-pei-i386.dll", 0x14c);
+
+          write_simple_dll ("linux-pei-i386.dll", 0x14c ^ 0x7b79 /* i386 + Linux */);
+        }
+      else if (strcmp ("pei-x86-64", wanted_format) == 0)
+        {
+          write_simple_dll ("simple-pei-x86-64.dll", 0x8664);
+
+          write_simple_dll ("linux-pei-x86-64.dll", 0x8664 ^ 0x7b79 /* x86-64 + Linux */);
+        }
+      else
+        {
+          fprintf (stderr, "error: can't handle format %s\n", wanted_format);
+          exit (INCORRECT_USAGE);
+        }
+    }

   return 0;
 }
--
2.21.0

============================================================
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Make dotnet assemblies test more robust

Omair Majid

Omair Majid <[hidden email]> writes:

> Omair Majid <[hidden email]> writes:
>
>> Nick Clifton <[hidden email]> writes:
>>
>>> I think that what we need to do is to extend gentestdlls.c so that
>>> it generates a normal x86 PE dll file as well.  Then the test can
>>> be extended to check this file first.  If the target objdump is
>>> able to parse this file, then it should be able to parse the other
>>> two files as well.  If it cannot then there is an error.  (Actually
>>> 32-bit PE toolchains may only be able to parse one of the new .NET
>>> PE dlls).
>>
>> Thanks for the idea! How does the attached patch look?
>
> Oops. I just realized that I made a mistake in strcmp. Fixed now.
>
> binutils/ChangeLog:
>
> 2019-07-25  Omair Majid  <[hidden email]>
>
> * testsuite/binutils-all/objdump.exp
> (test_objdump_dotnet_assemblies): Fix test to distinguish errors
> in parsing simple pei-i386 and pei-x86-64 vs parsing the newly
> introduced machine types.
> * testsuite/gentestdlls.c (write_simple_dll): New function.
> (main): Generate simple and Linux-specific variants of pei-i386
> and pei-x86-64 files so both can be used by tests.

Ping?

Thanks,
Omair

--
PGP Key: B157A9F0 (http://pgp.mit.edu/)
Fingerprint = 9DB5 2F0B FD3E C239 E108  E7BD DF99 7AF8 B157 A9F0
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Make dotnet assemblies test more robust

Nick Clifton
Hi Omair,

  Sorry - I have been off sick for the last week and a half. :-(

  Let me have a look over the patch and I will get back to you tomorrow.

Cheers
  Nick


Reply | Threaded
Open this post in threaded view
|

Re: RFC: Make dotnet assemblies test more robust

Omair Majid

Hi Nick,

Nick Clifton <[hidden email]> writes:

>   Sorry - I have been off sick for the last week and a half. :-(

Yikes. Sorry to hear that. Get well soon!

>   Let me have a look over the patch and I will get back to you tomorrow.

No worries. Your health is more important than a tiny patch! I just
pinged it to make sure it didn't fall between the cracks. Please don't
prioritize this over your health.

Regards,
Omair

--
PGP Key: B157A9F0 (http://pgp.mit.edu/)
Fingerprint = 9DB5 2F0B FD3E C239 E108  E7BD DF99 7AF8 B157 A9F0
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Make dotnet assemblies test more robust

Nick Clifton
Hi Omair,

> No worries. Your health is more important than a tiny patch! I just
> pinged it to make sure it didn't fall between the cracks. Please don't
> prioritize this over your health.

No worries - I am taking it easy, and I am on the mend.

Anyway, your patch applied cleanly, worked and did not introduce any
new testsuite failures to any other targets, so I was happy to apply it.
I did however make a couple of small changes:

  * I used the "return" keyword in the test_objdump_dotnet_assemblies
    proc in a few places in order to simplify the indentation and
    reduce brace nesting.

  * I added "pass $test" statements for the vanilla 32-bit and 64-bit
    PE format checks, just so that the success of these tests is
    recorded in the logs.

  * I added a couple of extra "set test ..." statements so that each
    specific test has its own descriptive name.

  * I added some comments to the proc so that any future readers are
    informed as to what is going on, and why the UNSUPPORTED results
    are not serious.

Cheers
  Nick