Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Parse variadic functions #2701

Merged
merged 9 commits into from
Nov 9, 2023
Merged

Parse variadic functions #2701

merged 9 commits into from
Nov 9, 2023

Conversation

P-E-P
Copy link
Member

@P-E-P P-E-P commented Oct 18, 2023 •
edited
Loading

Parse variadic functions.

This version of the PR modifies the way variadic are stored in the ast. Variadic were previously handled at function level, but this makes the parser a bit ugly. This PR suggest keeping variadic informations as a special parameter instead. It requires a lot of work to reflect this change at all level of the compiler. Many passes consider the number of arguments without checking for variadicity, therefore, this PR creates a big pile of ICE as of c6d9843.

Alternative to #2704

Fixes #2664
Fixes #2700

@P-E-P P-E-P added this to the GCC 14.1 release milestone Oct 18, 2023
@P-E-P P-E-P requested a review from CohenArthur October 18, 2023 11:33
@P-E-P P-E-P self-assigned this Oct 18, 2023
@P-E-P P-E-P force-pushed the parse-variadic-functions branch 2 times, most recently from e675b5e to bf6c4ec Compare October 18, 2023 13:38
@P-E-P P-E-P mentioned this pull request Oct 20, 2023
Closed
@P-E-P P-E-P force-pushed the parse-variadic-functions branch 5 times, most recently from 01dc98b to 385376e Compare October 24, 2023 10:30
@P-E-P P-E-P marked this pull request as ready for review October 24, 2023 11:33
@P-E-P
Copy link
Member Author

P-E-P commented Oct 24, 2023 •
edited
Loading

Although these changes pass the test, they are not entirely complete. We should error out when the variadic argument is incorrect. This should be handled in another PR.

Those bugs are described in issues #2711, #2712 and #2710

@P-E-P P-E-P requested a review from philberty October 30, 2023 14:02
philberty
philberty requested changes Nov 1, 2023
View reviewed changes
Copy link
Member

@philberty philberty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory it should be possibe to add a hello world test here by adding a parameter name to the printf varadic function lets make sure that works and go for it

philberty
philberty reviewed Nov 1, 2023
View reviewed changes
gcc/rust/ast/rust-item.h Outdated Show resolved Hide resolved
CohenArthur
CohenArthur approved these changes Nov 3, 2023
View reviewed changes
Copy link
Member

@CohenArthur CohenArthur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you! nice work :)

gcc/rust/ast/rust-ast.cc Outdated Show resolved Hide resolved
gcc/rust/parse/rust-parse-impl.h Outdated Show resolved Hide resolved
@P-E-P P-E-P force-pushed the parse-variadic-functions branch 2 times, most recently from 99af120 to b9e6943 Compare November 6, 2023 09:53
@P-E-P P-E-P requested a review from philberty November 6, 2023 09:54
@P-E-P
Copy link
Member Author

P-E-P commented Nov 6, 2023

In theory it should be possibe to add a hello world test here by adding a parameter name to the printf varadic function lets make sure that works and go for it

I did as you suggested, with two new tests in 171e3ae, one execute with a printf as well as another test to ensure named variadic with a wildcard pattern are parsed correctly.

P-E-P added 4 commits November 7, 2023 10:44
@P-E-P
Change FunctionParam to represent variadic params
c54ff36 
Variadic were represented at the function level while retaining most
informations of a given parameter.

gcc/rust/ChangeLog:

	* ast/rust-item.h (class FunctionParam): Add some informations to
	function parameters in order to be able to store variadic argument as
	a function parameter.

Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>
@P-E-P
Parse variadic functions
4f61181 
Variadic functions were not parsed because it is an unstable feature.
While it is still unstable, it is required in order to parse libcore.

gcc/rust/ChangeLog:

	* parse/rust-parse-impl.h (Parser::parse_function_param): Parse
	variadic functions.

Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>
@P-E-P
Add a function to check if a function is variadic
05c91fc 
This function provides an easy way to check for a function's varidicity.

gcc/rust/ChangeLog:

	* ast/rust-item.h: Add a getter to check if a given function is
	variadic.

Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>
@P-E-P
Allow variadic NamedFunctionParam
cbc406c 
This was made to align NamedFunctionParam with FunctionParam.

gcc/rust/ChangeLog:

	* ast/rust-item.h (class NamedFunctionParam): Add variadic boolean and
	another constructor.
	* hir/rust-ast-lower-extern.h: Avoid last parameter when variadic.

Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>
@P-E-P P-E-P force-pushed the parse-variadic-functions branch from 171e3ae to 52f448d Compare November 7, 2023 10:01
@P-E-P
Fix multiple issues with variadic representation
5f128e6 
The new variadic representation has introduced multiple issues and ICE
into the codebase. Some early passes in the compiler depend on the
parameters all having a type and being an actual parameter.

gcc/rust/ChangeLog:

	* ast/rust-ast.cc (ExternalFunctionItem::as_string): Adapt as_string
	function to the new ast representation.
	(NamedFunctionParam::as_string): Likewise.
	* ast/rust-item.h: Add a function to test whether a FunctionParam has
	a name pattern.
	* expand/rust-cfg-strip.cc (CfgStrip::visit): Adapt cfg strip visitor
	for the new variadic arguments.
	* hir/rust-ast-lower-extern.h: Adapt lowering to the new variadic
	function representation.
	* metadata/rust-export-metadata.cc (ExportContext::emit_function):
	Change call to constructor.
	* parse/rust-parse-impl.h (Parser::parse_named_function_param): Change
	NamedFunctionParam parsing to accomodate new variadic representation.
	(Parser::parse_external_item): Change external item parsing to use the
	new NamedFunctionParam variadics.
	* parse/rust-parse.h: Add new parsing function prototypes.
	* ast/rust-ast-collector.cc (TokenCollector::visit): Rework token
	collection to take into account variadic parameters.
	* ast/rust-ast-visitor.cc: Likewise.
	* expand/rust-expand-visitor.cc (ExpandVisitor::visit): Change function
	bound to avoid getting the type of a variadic parameter.
	* resolve/rust-ast-resolve-item.cc (ResolveExternItem::visit):
	Likewise.
	* resolve/rust-early-name-resolver.cc (EarlyNameResolver::visit):
	Likewise.

Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>
P-E-P added 4 commits November 7, 2023 13:07
@P-E-P
Add a new regression test
e5ea8be 
This new test highlight the behavior of the new parser and it's ability
to parse variadic rust functions.

gcc/testsuite/ChangeLog:

	* rust/compile/parse_variadic_function.rs: New test.

Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>
@P-E-P
Parse named variadic parameters
de05f00 
Add ability to parse named variadic parameters in extern c functions.

gcc/rust/ChangeLog:

	* parse/rust-parse-impl.h (Parser::parse_named_function_param): Add
	new parsing ability.

Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>
@P-E-P
Add a new regression test for named variadics
a6e0b5a 
This test ensure that extern C named variadics are parsed correctly.

gcc/testsuite/ChangeLog:

	* rust/compile/extern_c_named_variadic.rs: New test.

Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>
@P-E-P
Add named variadic argument test
e1d0970 
Variadic arguments may have a name or a pattern. This commit provides two
new tests in order to ensure their correct behavior.

gcc/testsuite/ChangeLog:

	* rust/compile/pattern_variadic.rs: New test.
	* rust/execute/torture/named_variadic.rs: New test.

Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>
@P-E-P P-E-P force-pushed the parse-variadic-functions branch from 52f448d to e1d0970 Compare November 7, 2023 12:07
philberty
philberty approved these changes Nov 9, 2023
View reviewed changes
Copy link
Member

@philberty philberty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM go for it

@philberty philberty added this pull request to the merge queue Nov 9, 2023
Merged via the queue into Rust-GCC:master with commit 25652e2 Nov 9, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CohenArthur CohenArthur CohenArthur approved these changes

@philberty philberty philberty approved these changes

Assignees

@P-E-P P-E-P

Labels
enhancement
Projects
libcore 1.49
Status: Done
Milestone
GCC 14.1 release
Development

Successfully merging this pull request may close these issues.

Cannot parse named variadic on extern functions Cannot parse variadic functions
3 participants
@P-E-P @philberty @CohenArthur