#2354 extern function prototypes improperly marked as public as well as extern

[alvin]

sdcc -v:
SDCC : mcs51/z80/z180/r2k/r3ka/gbz80/tlcs90/ds390/pic16/pic14/TININative/ds400/hc08/s08/stm8 3.4.3 #9167 (Feb 2 2015) (MINGW32)
published under GNU General Public License (GPL)
test1.c:

extern int ioctl(int fd, unsigned int request, ...);

void test1(void)
{
   ioctl(1, 0x1002, 0x000f);
}

Using asxxxx to generate output:

sdcc -mz80 --no-optsdcc-in-asm --c1mode < test1.c -o test1.asm

;--------------------------------------------------------
; Public variables in this module
;--------------------------------------------------------
    .globl _test1
    .globl _ioctl
;--------------------------------------------------------

Using z80asm to generate output:

sdcc -mz80 --no-optsdcc-in-asm --c1mode --asm=z80asm < test1.c -o test1.asm

;--------------------------------------------------------
; Public variables in this module
;--------------------------------------------------------
    XDEF _test1
    XDEF _ioctl
;--------------------------------------------------------
; Externals used
;--------------------------------------------------------
    XREF _ioctl
;--------------------------------------------------------

XDEF means PUBLIC and XREF means EXTERN.

PUBLIC (XDEF) indicates that the function is defined locally and the symbol is being exported. That is not the case for ioctl() whose definition is found externally. It should only be declared EXTERN rather than as both PUBLIC and EXTERN. sdcc is doing this for all prototypes declared extern from, eg, header files.

If the function has an extern prototype and the function is defined locally (as in header + implementation) then it should be declared both PUBLIC and EXTERN. But what sdcc is doing now is an error.

This error is covered up by taking advantage of asxxx's GLOBAL directive. The ports turn off extern declarations (the "externals used" section is not output when asxxx is the target assembler) and depend on marking all called functions as both PUBLIC and EXTERN. Then every function gets a GLOBAL directive from the PUBLIC list and the EXTERN list is omitted to prevent duplicates. asxxx attaches intelligence to GLOBAL so that if the symbol is not local it becomes EXTERN and if the symbol is local it becomes PUBLIC. So the bug is not apparent when using asxxx. For other assemblers without GLOBAL equivalent, the bug shows up as asm errors when the PUBLIC directive is attached to a function name not available in the source file.

I can see two ways to fix this -- the bug can be removed by marking these extern functions as EXTERN only rather than both PUBLIC and EXTERN. If that is done, all the port structures for asxxx will have to turn on generation of the EXTERN list (and I can only say that would work for the z80; maybe it's more complicated for other targets).

Another possibility is to insert a fix where the PUBLIC table is emitted at line 1683 in SDCCglue.c:

void
printPublics (FILE * afile)
{
  symbol *sym;

  fprintf (afile, "%s", iComments2);
  fprintf (afile, "; Public variables in this module\n");
  fprintf (afile, "%s", iComments2);

  for (sym = setFirstItem (publics); sym; sym = setNextItem (publics))
    if (! IS_EXTERN (sym->etype)) tfprintf (afile, "\t!global\n", sym->rname);
//      tfprintf (afile, "\t!global\n", sym->rname);  /*** this line replaced ***/
}

The condition should be something like "if generating extern variables list && (not in the extern list || (this is a non-static function implemented locally))" then output public symbol.

The !IS_EXTERN test I have there is working except in the case where the function is implemented locally (header + implementation case). I'm not sure how to do the correct conditional.

I'm asking for advice on the best way to fix this. We're patching sdcc to work with another z80 library (and assembler) and those patches may or may not be of interest to the sdcc project. If not, we will fork and do our own thing but if yes, any changes should have minimal impact on everything else.

[alvin]

The bug is at ~approx line 180 in SDCCglue.c inside function emitRegularMap()

The test to add to the public list is currently this:

    if ((sym->level == 0 ||
        (sym->_isparm && !IS_REGPARM (sym->etype) && !IS_STATIC (sym->localof->etype))) &&
        addPublics &&
        !IS_STATIC (sym->etype) &&
        (IS_FUNC (sym->type) ? (sym->used || IFFUNC_HASBODY (sym->type)) : (!IS_EXTERN (sym->etype) || sym->ival)) &&
        !(IFFUNC_ISINLINE (sym->type) && !IS_STATIC (sym->etype) && !IS_EXTERN (sym->etype)))

The error is in this conditional:

(IS_FUNC (sym->type) ? (sym->used || IFFUNC_HASBODY (sym->type))

If the symbol is a function, "sym->used" is also true if the function is called, ie also true in cases where the function has been declared extern but not also implemented locally.

The correction is to omit "sym->used" from this conditional:

(IS_FUNC (sym->type) ? (IFFUNC_HASBODY (sym->type))

When I do this, I get correct PUBLIC and EXTERN lists. However, I think that the asxxx assemblers are depending on this bug to generate GLOBALS from the public list, with the assembler taking care of the PUBLIC/EXTERN distinction.

So what I've decided to do is insert code that tests for the specific port I'm adding and doing the right thing for that port only.

[Maarten Brock]

AFAIR the ASxxxx assembler has no way to mark a symbol extern a.k.a. imported. Both the GLOBAL keyword and any label with double colons (label::) make the symbol public a.k.a. exported. There is only an option to treat all unknown symbols extern.

[alvin]

I did get sdcc generating code using z80asm but some of the changes were intrusive so we opted for a text post-processing step instead that takes the asz80-format z80 asm emitted by sdcc and translates it to standard zilog format to be consumed by z80asm.

However:

There is only an option to treat all unknown symbols extern.

This must be the mode asxxx is being used then in sdcc? I do not get any .globl declarations in the sdcc source for extern variables. Extern functions, on the other hand, do get a .globl declaration:

test.c

typedef unsigned char uint8_t;
typedef unsigned int uint16_t;

struct fzx_font
{
   uint8_t   height;
   uint8_t   tracking;
   uint8_t   last_char;
};


extern FILE *stdin;
extern struct fzx_font ff_ao_RoundelSans;

extern int fflush(FILE *stream);
extern int ioctl(int fd, uint16_t request, ...);
extern int printf(char *format, ...);

int i = 10;

int main()
{
   fflush(stdin);
   ioctl(1, 0x0802, &ff_ao_RoundelSans);
   printf("I = %d\n", i);

   return 0;
}

sdcc -mz80 -S test.c

;--------------------------------------------------------
; Public variables in this module
;--------------------------------------------------------
    .globl _main
    .globl _printf
    .globl _ioctl
    .globl _fflush
    .globl _i
....
_main:
;test.c:26: fflush(stdin);
    ld  hl,(_stdin)
    push    hl
    call    _fflush
;test.c:27: ioctl(1, 0x0802, &ff_ao_RoundelSans);
    ld  hl, #_ff_ao_RoundelSans
    ex  (sp),hl
    ld  hl,#0x0802
    push    hl
    ld  hl,#0x0001
    push    hl
    call    _ioctl
    ld  hl,#6
    add hl,sp
    ld  sp,hl
;test.c:28: printf("I = %d\n", i);
    ld  de,#___str_0
    ld  hl,(_i)
    push    hl
    push    de
    call    _printf
    pop af
    pop af
;test.c:30: return 0;
    ld  hl,#0x0000
    ret
_main_end::
___str_0:
    .ascii "I = %d"
    .db 0x0A
    .db 0x00
    .area _CODE
    .area _INITIALIZER

The extern data structures:

extern FILE *stdin;
extern struct fzx_font ff_ao_RoundelSans;

do not get any .globl declaration.

However the extern functions do:

extern int fflush(FILE *stream);
extern int ioctl(int fd, uint16_t request, ...);
extern int printf(char *format, ...);

    .globl _printf
    .globl _ioctl
    .globl _fflush

After post-processing we get symbol undefined errors for those extern data structures. Having read your comment it seems sdcc is depending on asxxx to treat undefined symbols as potentially extern. I don't know if we want to put that feature into z80asm as it could be error-prone. If we suggest a patch to have sdcc emit .globl for extern structures like it does for extern functions now would that break anything do you think?

[Maarten Brock]

I don't think that would break anything. Your patch is welcome.

[alvin]

We have a proposed patch under test and it seems to be working thus far.

I wanted to run it past you to see if it might be acceptable.

The patch can be had here:
https://drive.google.com/open?id=0B6XhJJ33xpOWdkhJWFdRdlo3bWs&authuser=0

We did two things that will be visible to targets other than the z80:

1. Extern functions are now in the externs list rather than the globals list. This means sdcc will no longer output a ".globl func" for extern functions. If the function is declared locally as in a header incude / implementation file, then it will appear in the globals list. I don't think this is a problem as this is how things work with extern variables.

2. The local label format is changed from "number$" to "l_functionname_number$". We had to do this because these local labels are not unique to a file. If the C source contains multiple independent functions, the translated asm will have local labels unique to only those functions and not the source file.

To the z80 target we added a "--emit-externs" command line flag. This sets
port->assembler.externGlobal to 1, meaning sdcc emits the externs list.

[Philipp Klaus Krause]

I applied the z80-specific parts of the patch in revision #9185.

The changes to SDCCasm.c break at least the ds390 port (I get an "undefined symbol during assembly" error when building the library).

The changes to SDCCglue.c break at least the hc08 port (I get a linker error in one of the regression tests).

Philipp

[alvin]

I was busy and was taken away from the project for a while.

I did find one problem in testing and that may be causing the undefined symbol errors. I will try to find some time to have a look.