Re: i386 code label alignment patch (version 2)

[Ian Lance Taylor <ian at cygnus dot com>]

   Date: Mon, 18 May 1998 16:44:04 -0400
   From: john@feith.com (John Wehle)

   > You seem to be looking at version numbers for what is essentially a
   > feature test.  I think you should run the feature test instead.  The
   > only case in which you need to check the version number is for a
   > Canadian Cross.  That should be a fallback, not the default.
   > 
   > In fact, I see that you actually run the assembler to get the version
   > number.  Instead, run the assembler to see if it supports what you
   > need.

   I can change this to use feature tests if desired.  It may been harder
   to reliably determine if a feature is available using feature tests
   instead of gas version numbers in some cases.  Feature tests certainly
   make sense if supporting features that are expected to be implemented
   in the same fashion by more than one assembler vendor.  At the moment
   I'm only concerned with taking advantage of newer gas specific features.
   It seems cleaner in this scenario to merely check GAS_MAJOR_VERSION and
   GAS_MINOR_VERSION.

I think feature tests are always better than version number tests.  I
think version number tests are a last resort.  That's because you
don't actually care what version number you are using.  You only care
whether a particular feature works.  You might be using some weird
mutant gas which thinks that it's version 970813 or something (and
believe me, there are plenty of such mutants out there).

   > You also seem to be confusing the issue of how to set the alignment
   > correctly with the issue of aligning code labels.  Those are separate
   > issues.  gcc should always be able to use ASM_OUTPUT_ALIGN.  The issue
   > of defining ASM_OUTPUT_ALIGN correctly is independent of the issue of
   > when to use it.  I think this patch will be easier to understand if
   > you break it into two independent patches.

   I don't mean to be confusing them.  You're right that changing
   ASM_OUTPUT_ALIGN is not directly related to aligning code labels, except
   for the fact that them both rely on knowing what forms of .p2align
   are supported by the assembler.  I can certainly remove the
   ASM_OUTPUT_ALIGN change from the patch if desired.

My point is that ASM_OUTPUT_ALIGN should always work.  I don't think
you should discard any part of the patch.  It's just that you seem to
have two independent patches combined into one.  One patch fixes
ASM_OUTPUT_ALIGN to be right.  The other uses it in some fashion when
generating x86 code.  Both patches are probably basically right.  But
they are different, and should not be mixed in the same patch.

   >  + elif [[ -f ../gas/Makefile.in ]]; then
   >  + 	gcc_cv_gas_major_version=`sed -n 's/^VERSION=\([[0-9]]*\)\..*/\1/p' < ../gas/Makefile.in`
   >  + 	gcc_cv_gas_minor_version=`sed -n 's/^VERSION=[[0-9]]*\.\([[0-9]]*\).*/\1/p' < ../gas/Makefile.in`
   > 
   > That does not work with the current gas sources.  As of binutils 2.9,
   > the version number is actually found in gas/configure.in, not
   > gas/Makefile.in.

   Actually I changed this to ../gas/Makefile after sending out version
   2 of the patch since I believe that ../gas/Makefile.in only works if
   srcdir == objdir.  It looks like binutils 2.9.1 still has VERSION in
   gas/Makefile even though it's not set in gas/Makefile.in.  Does this
   sound correct / how would you suggest doing this?

Yes, that sounds right, though fragile.  Since you are checking the
version number at configure time, this will only work if gas is
configured before gcc, since otherwise gas/Makefile won't exist.  I
think it would be more reliable to try to pick up the version number
from gas/configure.in, and if that fails, then from gas/Makefile.in.
And of course I think you should only do this test in the Canadian
Cross case.

Ian