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: RFC: Add initial support for .NET Core dlls to objdump


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


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