#4122 PIC16: avoid exporting non-static inline functions as public symbols

[zeynelsagcan]

The PIC16 backend currently adds non-static inline functions to the public symbol set even when no externally visible out-of-line definition is emitted.
This can result in unnecessary or invalid public symbol declarations in the generated assembly.
This patch prevents non-static, non-extern inline functions from being added to the PIC16 public symbol set, while preserving the existing behavior for normal externally visible functions.
Tested with the PIC16 backend on SVN r16960.
This patch is independent of the PIC16 COFF debug metadata changes submitted separately.

[Philipp Klaus Krause]

The attached patch file looks empty to me. Is it supposed to be the same as the one over at [bugs:#4110]?

P.S.: That one has been applied now.

[zeynelsagcan]

Thanks for pointing this out. I downloaded the attachment from #4122 and confirmed that it is indeed empty.

No, this is a separate fix from #4110. Issue #4110 concerns stdbit.h / _BitInt, whereas #4122 fixes the PIC16 backend incorrectly adding non-static, non-extern inline functions to the public symbol list when no external definition is emitted.

I will replace the empty attachment with the correct patch. Sorry for the confusion.

[Philipp Klaus Krause]

Over at [#4110], there were two patches attached. Both are applied now. One was about inline functions.

[Maarten Brock]

I am wondering what global actually does and whether it's gpasm or gplink that complains.

• Does it export the symbol externally in gpasm saying it is defined here?
• Or does it import the symbol telling the linker it should be defined elsewhere?

In the latter case it should be the linker complaining that it cannot find the definition in the objects passed or its libraries. And this is correct behaviour for an inline definition. When using function pointers inlining is not possible and thus the external definition must be used.

unsigned char bar (unsigned char (*fp)(unsigned char value, unsigned int count), unsigned char value, unsigned int count)
{
  return fp(value, count);
}

void foo(void)
{
  bar(stdc_rotate_left_uc, 1, 2);
}

[Philipp Klaus Krause]

While I have no real idea, why pci16 does or did what it did: pic16/glue.c behaves differently from the rest (SDCCglue.c and pic14/glue.c). With the patch applied it is a bit more like the rest. In the end our problem is that we have no pic experts among SDCC developers, and no pic port maintainer.

[zeynelsagcan]

I repeated the function-pointer test with a clean build of SDCC trunk r16997, after the r16990 change, targeting PIC18F452.

The generated assembly still references _stdc_rotate_left_uc using UPPER(), HIGH(), and LOW(), but does not emit either global or extern for this symbol.

GPASM fails with:

pic_test.asm:61:Error[113] Symbol not previously defined: "_stdc_rotate_left_uc"
pic_test.asm:63:Error[113] Symbol not previously defined: "_stdc_rotate_left_uc"
pic_test.asm:65:Error[113] Symbol not previously defined: "_stdc_rotate_left_uc"

Adding only:

extern _stdc_rotate_left_uc

makes GPASM succeed (GPASM_R16997_EXTERN_EXIT=0).

Therefore, the missing external declaration remains reproducible in r16997.

This appears to be a separate issue from the original #4122 fix: the patch prevents exporting non-static inline definitions, while taking the function's address still requires the assembler to recognize an external reference. If no external definition is supplied, the unresolved-symbol diagnostic should occur at link time.

[zeynelsagcan]

I tested this with SDCC 4.6.4 #16960 and gpasm/gplink 1.5.2 #1333, targeting PIC18F452.

The original assembly generated by SDCC contains:

global _bar
global _foo

There is no global or extern declaration for _stdc_rotate_left_uc, although its address is referenced as follows:

MOVLW UPPER(_stdc_rotate_left_uc)
MOVLW HIGH(_stdc_rotate_left_uc)
MOVLW LOW(_stdc_rotate_left_uc)

I tested three cases:

1. No declaration

GPASM reports:

Error[113] Symbol not previously defined: "_stdc_rotate_left_uc"

2. Adding global _stdc_rotate_left_uc

GPASM still fails. The global directive itself produces Error[113], followed by the same errors at the three address references.

3. Adding extern _stdc_rotate_left_uc instead

GPASM succeeds (GPASM_EXTERN_EXIT=0).

When linking the resulting object without providing an external definition, GPLINK reports:

error: External symbol "_stdc_rotate_left_uc" in section "S_pic_test__foo" not found.

This confirms that global exports a symbol defined in the current module, whereas extern declares a reference to a symbol defined elsewhere.

The distinction regarding C inline semantics is important: taking the address of an inline function can require an externally linked definition, and GPLINK should report an unresolved symbol if no such definition is supplied.

However, that is separate from the assembler-level issue: SDCC generates references to the function address without emitting the corresponding extern declaration, causing GPASM to fail before linking.

These tests were performed with r16960. I have not yet verified the behavior after the r16990 changes.

[zeynelsagcan]

PIC16: Tested patch for missing extern declaration (r16997)

I have attached a seven-line patch for src/pic16/gen.c, together with a minimal two-translation-unit reproducer.

Unpatched SDCC r16997:

• Generates references to _twice without declaring it as an external symbol.
• GPASM fails with three Error[113]: Symbol not previously defined errors.

Patched SDCC r16997:

• Automatically emits extern _twice.
• Both translation units compile and assemble successfully.
• GPLINK resolves the external symbol and successfully generates a 632-byte HEX file.

The patch registers a non-static inline function in the PIC16 externs set when it is processed as a function operand.

Additional regression tests passed for static inline functions, ordinary functions, direct inline calls, unused inline functions, and C99/C11/C23/C2y modes.

The attachment contains the patch and both C source files.

I would appreciate feedback on whether pic16_aopOp() is the preferred location for this fix or whether the external symbol should be registered in glue.c instead.

[zeynelsagcan]

Additional regression test: local function pointer initialization

I also tested an address-taken non-static inline function initialized into a local function pointer:

inline unsigned char twice(unsigned char x)
{
    return x + x;
}

unsigned char test(void)
{
    unsigned char (*fp)(unsigned char) = twice;
    return fp(5);
}

With unpatched SDCC r16997, assembly generation succeeds, but GPASM reports three Error[113] errors for _twice, because the required extern declaration is missing.

With the proposed seven-line PIC16 patch, extern _twice is emitted and compilation through GPASM succeeds.

As a control, I also tested initialization of a global function pointer. Both original and patched r16997 handle that case correctly, emitting extern _twice.

This confirms that the issue affects at least two runtime function-address uses: passing an inline function as an argument and initializing a local function pointer. Global function-pointer initialization is already handled by the existing PIC16 code.

[Maarten Brock]

Since SDCC handles this in SDCCglue.c emitRegularMap() for all non-pic targets, I suggest to fix this in glue.c.

/* if global variable & not static or extern
   and addPublics allowed then add it to the public set */
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)))
  {
    addSetHead (&publics, sym);
  }

/* if extern then add it into the extern list */
if (IS_EXTERN (sym->etype) && !sym->ival)
  {
    addSetHead (&externs, sym);
    continue;
  }

[zeynelsagcan]

Thank you, Maarten. I moved the fix from src/pic16/gen.c to src/pic16/glue.c, inside pic16emitRegularMap(), as suggested.

The new patch adds used, non-static, non-extern inline function symbols to the externs set.

I tested the change with SDCC trunk r16997, PIC18F452, and gputils:

• Taking the address of an external-linkage inline function correctly emits extern _twice.
• GPASM succeeds without Error[113].
• The two-translation-unit test links successfully with GPLINK and produces a 632-byte HEX file.
• A direct inline call does not generate an unnecessary extern.
• Taking the address of a static inline function keeps its local definition without generating an extern.
• An unused inline function does not generate an extern.

The fix works with the original, unmodified gen.c. The clean patch also passes git apply --check.

I have not yet run the full PIC16 regression suite. The updated glue.c patch is attached for review.