Skip to content

fix(extract): a C-family function's return type keeps its pointer and qualifiers - #2274

Open
DeusData wants to merge 1 commit into
mainfrom
distill/1245-c-pointer-return-types
Open

DeusData wants to merge 1 commit into
mainfrom
distill/1245-c-pointer-return-types

Conversation

@DeusData

Copy link
Copy Markdown
Owner

No C or C++ function in any index had a pointer return type. Distilled from #1245 (Andrew Hundt, the extract_defs.c half of c51e5cb; the mcp.c and token-reduction parts are not taken).

The defect

For C-family functions return_type was the text of the declaration's type node alone. In these grammars the *, & and && live on the declarator and cv-qualifiers are sibling nodes, so

const char *get_name(void)        published   char
robj **lookupKeys(...)            published   robj
Text &operator=(const Text &)     published   Text

The C/C++ resolver's return-type parser already strips a leading const/volatile and handles trailing *, &, && — branches that were unreachable for functions, so every Foo *f() was registered as returning Foo by value.

The fix

c_declared_return_type() renders the declared type at both sites (functions and methods) for C, C++, CUDA, GLSL, HLSL, ISPC, Slang and Objective-C — the list resolve_func_name_c_family uses:

<leading cv-qualifiers, source order> <base type, verbatim><one space><markers, outermost first>

so const char *, char **, Text &, Text &&, Text *&, volatile unsigned long *; a pointer-level qualifier follows its star (char *const *); east-const char const * normalises to const char *. With no qualifier and no marker the old text is returned unchanged. It measures, then writes into one exact-size arena allocation; the declarator walk is a plain child chain, no recursion. def.return_types is untouched.

Two deliberate differences from the upstream hunk. Upstream copied every type_qualifier node, which in these grammars includes constexpr, _Noreturn, mutable and __extension__constexpr int f() would have become constexpr int. Only const, volatile, restrict, __restrict, __restrict__ and _Atomic are kept, and both cases are asserted unchanged. And the walk stops at the first declarator that is neither pointer nor reference, so int (*f(void))(int) stays int rather than becoming a wrong int *.

RED → GREEN

RED with production reverted and the final tests kept:

c_function_return_type_preserves_pointer_and_qualifier    FAIL "char" != "const char *"
cpp_method_return_type_preserves_pointer_and_qualifier    FAIL "char" != "const char *"

c_function_return_type_plain_unchanged is the control and passes before and after. GREEN on today's main: extraction 374, c_lsp 762, pipeline 286, lang_contract 41; memory-core linter unchanged.

Effect on real code — Redis, production binaries, old against new

39,132 nodes in both. 1,442 Function nodes change, in return_type only: void → void * (270), char → const char * (101), unsigned char → unsigned char * (91), char → char * (81), robj → robj * (57), … Functions with a * in return_type: 0 → 1,440. With one worker the complete edge set apart from SEMANTICALLY_RELATED is byte-identical old against new across six runs — CALLS, USAGE, WRITES, IMPORTS and the rest do not move. SEMANTICALLY_RELATED goes 350 → 325 because that pass tokenises the return-type string into its type vector.

One thing measured rather than assumed: with several workers, which dict.h Redis' src/dict.c imports depends on worker merge order. That was main's defect, not this change's, and it is what #2227 fixed.

… qualifiers

Distilled from #1245 by Andrew Hundt (c51e5cb, the extract_defs.c
half; the mcp.c and token-reduction parts are not taken).

For C-family functions return_type was the text of the declaration's
`type` node alone. In these grammars the `*`, `&` and `&&` live on the
declarator and cv-qualifiers are sibling nodes, so

    const char *get_name(void)     published   char
    robj **lookupKeys(...)         published   robj
    Text &operator=(const Text &)  published   Text

and no C or C++ function in any index had a pointer return type at all.
The C/C++ resolver's return-type parser already strips a leading
const/volatile and handles trailing `*`, `&`, `&&` -- branches that were
unreachable for functions, so every `Foo *f()` was registered as
returning Foo by value.

c_declared_return_type() renders the declared type at both sites
(functions and methods) for C, C++, CUDA, GLSL, HLSL, ISPC, Slang and
Objective-C -- the list resolve_func_name_c_family uses:

    <leading cv-qualifiers, source order> <base type, verbatim>
    <one space><markers, outermost first>

so `const char *`, `char **`, `Text &`, `Text &&`, `Text *&`,
`volatile unsigned long *`; a pointer-level qualifier follows its star
(`char *const *`); east-const `char const *` normalises to
`const char *`. With no qualifier and no marker the old text is returned
unchanged. It measures, then writes into one exact-size arena
allocation; the declarator walk is a plain child chain, no recursion.
def.return_types is untouched.

Two deliberate differences from the upstream hunk. Upstream copied every
type_qualifier node, which in these grammars includes constexpr,
_Noreturn, mutable and __extension__ -- `constexpr int f()` would have
become `constexpr int`. Only const, volatile, restrict, __restrict,
__restrict__ and _Atomic are kept, and both cases are asserted unchanged.
And the walk stops at the first declarator that is neither pointer nor
reference, so `int (*f(void))(int)` stays `int` rather than becoming a
wrong `int *`.

RED, production reverted and the final tests kept:
  c_function_return_type_preserves_pointer_and_qualifier
      FAIL tests/test_extraction.c:1005: "char" != "const char *"
  cpp_method_return_type_preserves_pointer_and_qualifier
      FAIL tests/test_extraction.c:1072: "char" != "const char *"
  351 passed, 2 failed
c_function_return_type_plain_unchanged is the control: it passes before
and after.

GREEN: extraction 353, c_lsp 762, lang_contract 41, pipeline 281.

Effect on real code -- Redis, production binaries, old against new.
39,132 nodes in both. 1,442 Function nodes change, in return_type only:
void -> void * (270), char -> const char * (101), unsigned char ->
unsigned char * (91), char -> char * (81), robj -> robj * (57), ...
Functions with a `*` in return_type: 0 -> 1,440. With one worker the
complete edge set apart from SEMANTICALLY_RELATED is byte-identical old
against new across six runs (three each) -- CALLS, USAGE, WRITES, IMPORTS
and the rest do not move. SEMANTICALLY_RELATED goes 350 -> 325 because
that pass tokenises the return-type string into its type vector.

One thing measured rather than assumed. With several workers, which
`dict.h` Redis' src/dict.c imports (src/ or deps/hiredis/) depends on
worker merge order. That is main's defect, not this change's: unmodified
main picked src/dict.h in 2 of 10 runs and the hiredis header in 8; with
one worker both binaries always pick src/dict.h. This change picked
src/dict.h in 6 of 10. At ten runs a side that gap is not separable from
chance (Fisher exact, two-sided p = 0.17), and a shifted rate would be
unsurprising for a timing-dependent pick; it is recorded here rather
than explained. The include-target fix is a separate change (#2227).

Known limits, all pre-existing and out of scope: struct and class FIELDS
have the same defect (`const char *data;` publishes char); C prototypes
and declaration-only C++ methods are not extracted as definitions, so
they are not reached; where a macro is misparsed as the type
(`LUA_API const char *f()`), the base was already wrong and now reads
`const LUA_API *`; the resolver's parser does not yet understand a
pointer-level qualifier or a leading _Atomic/restrict and falls back to
a named-type lookup for those, as it effectively did for every pointer
return before.

Co-authored-by: Andrew Hundt <ATHundt@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant