Build on sparc calls assembler incorrectly

Review Request #1376 — Created Jan. 17, 2019 and submitted

ptribble
illumos-gate
10257
general
Building on sparc fails with many of the following in libbc:

/usr/ccs/bin/as -xregsym=no -P -D__STDC__ -DLOCORE -D_SYS_SYS_S -D_ASM -Dsparc
-I. -Iinc -I../inc/include -I../inc/include/sys -DTEXT_DOMAIN=\"SUNW_OST_OSLIB\"
-D_TS_ERRNO -I/export/home/ptribble/Illumos/test1-gate/proto/root_sparc/usr/include -fpic -DPIC -K pic -DPIC ../libc/crt/sparc/misalign.s -o pics/misalign.o
/usr/ccs/bin/as: error: unknown option 'f'
/usr/ccs/bin/as: error: unknown option 'p'

Looking at the Makefile, CPPFLAGS is augmented to contain -fpic via C_PICFLAGS, which is then copied to ASFLAGS. So simply copy the original CPPFLAGS into ASFLAGS, so it can't get modified wrongly later.

With the fix, build on sparc no longer has the error.

I can also verify, as of revision 2, that the actual flags passed to as are the same as they were before the change that broke this. (I have an old nightly.log for comparison.)

  • 0
  • 0
  • 0
  • 1
  • 1
Description From Last Updated
jlevon
  1. 
      
  2. usr/src/lib/libbc/sparc/Makefile (Diff revision 1)
     
     

    You could just define a BC_CPPFLAGS, then reference
    it from both $(CPPFLAGS) and $(ASFLAGS). Don't think you'd need the comment then, and there's no duplication

    1. Moving all the -I flags to "INCDIRS" would also help cut down on the duplication.

    2. Using INCDIRS won't work as it's global; different files here need building with different includes.

      There's no real benefit to setting BC_CPPFLAGS; I would still add the comment in that case to explain why it wasn't just using CPPFLAGS. I would prefer not to, as this is horribly fragile.

    3. I see "-I. -Iinc -I../inc/include -I../inc/include/sys" in both ASFLAGS and CPPFLAGS. If you separate it out into "INCDIRS" you only need to add "$(INCDIRS)" to ASFLAGS and CPPFLAGS. Of course you can name the variable whatever you like, but "INCDIRS" seems to be the common name.

    4. Overloading INCDIRS specifically risks leakage, it would have to be an unused name to guarantee it didn't get mangled.

      Yes, you could optimize this, but it's grotty code in the first place, and given the likelihood that we rip the whole lot out at some point spending too much effort on improving something when the current solution is known to work seems undesirable.

    5. INCDIRS is purely a convention. You'd not be overloading anything.

  3. 
      
ptribble
ptribble
jlevon
  1. Think I agree with Peter at this point, too much energy expended on old crap...

  2. 
      
domag02
  1. Ship it!

    Just out of curiosity, is it better?:

  2. usr/src/lib/libbc/sparc/Makefile (Diff revision 2)
     
     

    BC_DEFS= -Dsparc
    BC_S5DEFS= $(BC_DEFS) -DS5EMUL
    
    BC_AS_DEFS= -D__STDC__ -DLOCORE -D_SYS_SYS_S -D_ASM $(BC_DEFS)
    BC_AS_S5DEFS= $(BC_AS_DEFS) -DS5EMUL
    
    BC_INCS= -I. -Iinc -I../inc/include -I../inc/include/sys
    BC_S5INCS= -I. -Iinc -I../inc/5include -I../inc/include -I../inc/include/sys
    
    CPPFLAGS.first= $(BC_DEFS) $(BC_INCS)
    ASFLAGS= -P $(BC_AS_DEFS) $(BC_INCS) $(CPPFLAGS.master)
    
  3. usr/src/lib/libbc/sparc/Makefile (Diff revision 2)
     
     

    # conditional assignments
    s5pics/%.o:= CPPFLAGS = $(BC_S5DEFS) $(C_PICFLAGS) -DPIC \
        $(BC_S5INCS) $(CPPFLAGS.master)
    s5pics/%.o:= ASFLAGS = -P $(BC_AS_S5DEFS) $(AS_PICFLAGS) -DPIC \
        $(BC_S5INCS) $(CPPFLAGS.master)
    

    Or with CPPFLAGS.first?:

    # conditional assignments
    s5pics/%.o:= CPPFLAGS.first = $(BC_S5DEFS) $(C_PICFLAGS) -DPIC \
        $(BC_S5INCS)
    s5pics/%.o:= ASFLAGS = -P $(BC_AS_S5DEFS) $(AS_PICFLAGS) -DPIC \
        $(BC_S5INCS) $(CPPFLAGS.master)
    
  4. 
      
andy_js
  1. Ship It!
  2. 
      
ptribble
Review request changed

Status: Closed (submitted)

Loading...