Re: [3.1.1] x86-64 fixes

[Mark Mitchell <mark at codesourcery dot com>]

--On Wednesday, May 22, 2002 03:58:36 PM +0200 Jan Hubicka <jh@suse.cz> 
wrote:

> Hi,
> the attached patches as accumulated in our tree. All of them are x86-64
> specific and has been extensivly tested in both i386 and x86-64
> compilers.  OK for 3.1.1?

Not without more explanation.

In general, each of these patches is under-commented.

Tue May 14 12:32:10 CEST 2002  Jan Hubicka  <jh@suse.cz>
	* dwarf2out.c (output_call_frame_info): Do not skip unwind info
	when flag_asynchronous_unwind_tables is set.

Fine, but why?  The comment says:

        /* Don't emit EH unwind info for leaf functions that don't need
           it.  */

The point is that this comment is vacuous; it says "we don't X if we
don't need X".  The comment should explain *why* the condition means
that we don't need X.  Something like:

  /* If this frame information is only used for exception handling
     (not debugging), and this function cannot throw, there's no
     need for the exception handling information; nothing can use
     it.  But, if we are using aysnchronous something-or-other, then
     we *do* need the information; we might get an exception in the
     midst of something and need to do something else.  */

I know you didn't write it, but you can improve it. :-)

In addition, you haven't explained whether this change fixes a regression.

The i386-protos.h/i386.c/unix.h change doesn't have a ChangeLog.  It's
great to have create a function for thunk output, but I can't tell
if anything changed from the two header file versions.  Also, you need
comments for each of the parameters to the function.  Finally, the
function has not a single comment in it; couldn't we at least have
a template for the code we're going to generate in each of the
conditions:

  if (TARGET_64BIT)
    {
      /* Generate code that looks like:

           add %eax, $18  ;; Adjust the this pointer.
           jmp f          ;; Jump to the called function.

         If we're generating PIC code, then the jump is done with
         a PC-relative branch:

           jmp @GOTPCREL(%rip)

         instead.  */
    }

Again, is this a regression?

The last change says it "copies" another version of the same macro.  If
that's true, put it somewhere common so it can be shared.  And why do
we do "ifdef __x86_64__"; isn't that testing something about the
host platform, rather than the target platform?

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com