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

Inline asm #2982

Draft
wants to merge 28 commits into
base: master
Choose a base branch
from
Draft

Inline asm #2982

wants to merge 28 commits into from
+866 −18

Conversation

badumbatish
Copy link
Contributor

Draft PR for my mentors

@badumbatish badumbatish force-pushed the inline_asm branch 6 times, most recently from 5a0f0a9 to 69a06ce Compare May 22, 2024 04:28
CohenArthur
CohenArthur reviewed May 24, 2024
View reviewed changes
gcc/rust/ast/rust-expr.h Outdated Show resolved Hide resolved
gcc/rust/ast/rust-expr.h Outdated Show resolved Hide resolved
gcc/rust/expand/rust-macro-builtins.cc Outdated Show resolved Hide resolved
@badumbatish badumbatish force-pushed the inline_asm branch 3 times, most recently from 6012f81 to 5af029d Compare May 29, 2024 23:10
@badumbatish
Almost done with top level parsing
d55eddb 
gcc/rust/ChangeLog:

	* expand/rust-macro-builtins-asm.cc (struct AsmParseError): title.
	(enum InlineAsmDirSpec): title.
	(enum InlineAsmOptions): title.
	(struct AsmArg): title.
	(parseAsmArg): title.
	(parse_global_asm): title.
	(parse_nonglobal_asm): title.
	(parse_asm): title.
	(parseDirSpec): title.
	(parse_format_string): title.
	(MacroBuiltin::global_asm_handler): title.
	(MacroBuiltin::nonglobal_asm_handler): title.
	* expand/rust-macro-builtins.cc: title.
	* expand/rust-macro-builtins.h: title.
@badumbatish
Working on parseAsmArg
95acee0 
gcc/rust/ChangeLog:

	* expand/rust-macro-builtins-asm.cc (enum InlineAsmRegOrRegClass): title.
	(parseAsmArg): title.
	(check_identifier): title.
	(parse_operand): title.
	(parse_options): title.
	(parse_reg): title.
	(parseDirSpec): title.
	(parse_asm): title.
@badumbatish
Introduce first implementation of parse_clobber_abi
3e0f614 
gcc/rust/ChangeLog:

	* expand/rust-macro-builtins-asm.cc (parse_clobber_abi): title.
	(parseAsmArg): title.
	* expand/rust-macro-builtins-asm.h (parse_clobber_abi): title.
@badumbatish
Working on parse_options for a bit more
706fe7e 
gcc/rust/ChangeLog:

	* expand/rust-macro-builtins-asm.cc (parse_options): title.
@badumbatish
Fix clobber_api brackets
fbaa878 
gcc/rust/ChangeLog:

	* expand/rust-macro-builtins-asm.cc (parse_clobber_abi): title.
@badumbatish
First draft of parse_option finished
4203f02 
Finish up on parse_option, formatted parse_clobber_abi
gcc/rust/ChangeLog:

	* expand/rust-macro-builtins-asm.cc (parse_clobber_abi): format
	(check_and_set): helper function, is try_set_option equivalent
	(parse_options): new function
	* expand/rust-macro-builtins-asm.h (enum InlineAsmOptions):
	removed
	(check_and_set): decl of helper function
@badumbatish
Introduced is_global_asm to InlineAsm AST
194986c 
gcc/rust/ChangeLog:

	* ast/rust-expr.h: Introduced is_global_asm to InlineAsm AST
@badumbatish
Make InlineAsm non-abstract for usage in parsing.
876d4e7 
gcc/rust/ChangeLog:

	* ast/rust-expr.h: Make InlineAsm non-abstract for usage in parsing.
@badumbatish
Replace scaffolded InlineAsm with real InlineAsm.
fa9882e 
Replace scaffolded InlineAsm with real InlineAsm node in rust-expr.h

gcc/rust/ChangeLog:

	* expand/rust-macro-builtins-asm.cc (parseDirSpec): replace
	scaffolded InlineAsm with real InlineAsm.
	(parse_clobber_abi): likewise.
	(check_and_set): likewise.
	(parse_options): likewise.
	(parseAsmArg): likewise.
	(parse_asm): likewise.
	* expand/rust-macro-builtins-asm.h (struct AsmParseError): likewise.
	(enum InlineAsmDirSpec): likewise.
	(enum InlineAsmRegOrRegClass): likewise.
	(struct AsmArg): likewise.
	(parseAsmArg): likewise.
	(check_and_set): likewise.
	(parse_operand): likewise.
	(parse_options): likewise.
	(parse_reg): likewise.
	(parse_clobber_abi): likewise.
@badumbatish
Add a simple no-op test for asm!
9810e84 
gcc/testsuite/ChangeLog:

	* rust/compile/inline_asm_nop.rs: Simple test for asm!
@badumbatish
Top level parsing test for asm!
f3002e3 
gcc/testsuite/ChangeLog:

	* rust/compile/inline_asm_ident_first.rs: New test.
@badumbatish
Add tests parsing with trailing commas inline asm
20b823b 
gcc/testsuite/ChangeLog:

	* rust/compile/inline_asm_nop_2.rs: New test.
@badumbatish
Added faulty tests for inline asm cloberring
d916294 
gcc/testsuite/ChangeLog:

	* rust/compile/inline_asm_faulty_clobber.rs: New test.
	* rust/compile/inline_asm_faulty_clobber_1.rs: New test.
	* rust/compile/inline_asm_faulty_clobber_2.rs: New test.
@badumbatish
Implemented parse_clobber_abi to pass new tests
80f4f3d 
gcc/rust/ChangeLog:

	* expand/rust-macro-builtins-asm.cc (parse_clobber_abi):
	implemented parse_clobber_abi
	(parse_format_string): likewise.
	(parseAsmArg): likewise.
	(parse_asm): likewise.
	* expand/rust-macro-builtins-asm.h (parseAsmArg): likewise.
@badumbatish
Parsing of options(...) done.
dcd684f 
This is without any mutually exclusive options checked, or
any relationship with reg_operands. Very primitive.

gcc/rust/ChangeLog:

	* ast/rust-expr.h: parsing of options(...)
	* expand/rust-macro-builtins-asm.cc (check_and_set):
	likewise.
	(parse_options): likewise.
	(parseAsmArg): likewise.
	* expand/rust-macro-builtins-asm.h (check_and_set):
	likewise.

gcc/testsuite/ChangeLog:

	* rust/compile/inline_asm_legal_options.rs: New test.
@badumbatish
Wraps inline_asm tests in unsafe {}
1079477 
gcc/testsuite/ChangeLog:

	* rust/compile/inline_asm_faulty_clobber.rs:
	Wraps inline_asm tests in unsafe {}
	* rust/compile/inline_asm_faulty_clobber_1.rs: likewise.
	* rust/compile/inline_asm_faulty_clobber_2.rs: likewise.
	* rust/compile/inline_asm_ident_first.rs: likewise.
	* rust/compile/inline_asm_nop.rs: likewise.
	* rust/compile/inline_asm_nop_2.rs: likewise.
@badumbatish
Add checks for duplicate options in inline asm
ca6a5b6 
gcc/rust/ChangeLog:

	* expand/rust-macro-builtins-asm.cc (check_and_set):
	Add checks for duplicate options in inline asm
	(parse_options): likewise.

gcc/testsuite/ChangeLog:

	* rust/compile/inline_asm_illegal_options.rs: New test.
@badumbatish
Resolve static decl warning
afb199e 
gcc/rust/ChangeLog:

	* expand/rust-macro-builtins-asm.h (parseAsmArg):
	Resolve static decl warning
	(parse_nonglobal_asm):
	Resolve static decl warning
@badumbatish
Scaffolding parse_reg
5dc97dd 
gcc/rust/ChangeLog:

	* ast/rust-expr.h (struct InlineAsmRegOrRegClass):
	Scaffolding parse_reg
	* expand/rust-macro-builtins-asm.cc (parse_reg): Likewise.
	(parse_operand): Likewise.
	(parseAsmArg): Likewise.
@badumbatish
Rename InlineAsmOptions to InlineAsmOption
ea55d28 
Rename InlineAsmOptions to InlineAsmOption for clarity

gcc/rust/ChangeLog:

	* ast/rust-ast-full-decls.h (enum class):
	Rename InlineAsmOptions to InlineAsmOption for clarity
	* ast/rust-expr.h (enum class): Likewise.
	* expand/rust-macro-builtins-asm.cc (check_and_set): Likewise.
	(parse_options): Likewise.
	* expand/rust-macro-builtins-asm.h (check_and_set): Likewise.
	* hir/tree/rust-hir-expr.h: Likewise.
@badumbatish
Safegaurd InlineAsm's clone_expr_...
0ba05d5 
Safegaurd InlineAsm's clone_expr_... with unreachable since
we would never use them.

gcc/rust/ChangeLog:

	* ast/rust-expr.h:
	Safegaurd InlineAsm's clone_expr_... with unreachable.
@badumbatish
Remove global_asm and non_global_asm handler
9eba3bc 
gcc/rust/ChangeLog:

	* expand/rust-macro-builtins-asm.cc
	(MacroBuiltin::global_asm_handler):
	Remove global_asm and non_global_asm handler
	(MacroBuiltin::nonglobal_asm_handler): Likewise.
	(MacroBuiltin::asm_handler): Likewise.
	* expand/rust-macro-builtins.cc (enum class): Likewise.
	(inline_asm_maker): Likewise.
	* expand/rust-macro-builtins.h: Likewise.
@badumbatish
Big Refactor after meeting
6de9440 
gcc/rust/ChangeLog:

	* ast/rust-expr.h (struct InlineAsmRegOrRegClass):
	Remove union
	(struct InlineAsmOperand): Make instances of inside struct.
	* expand/rust-macro-builtins-asm.cc (parse_clobber_abi):
	Change signature with inlineAsmCtx.
	(parse_reg): Likewise.
	(parse_operand): Likewise.
	(parse_reg_operand): Likewise.
	(check_and_set): Likewise.
	(parse_options): Likewise.
	(parse_format_string): Likewise.
	(parseAsmArg): Likewise.
	(parse_asm): Likewise.
	* expand/rust-macro-builtins-asm.h (class InlineAsmContext): Likewise.
	(parseAsmArg): Likewise.
	(check_and_set): Likewise.
	(parse_reg_operand): Likewise.
	(parse_options): Likewise.
	(parse_reg): Likewise.
	(parse_clobber_abi): Likewise.
@badumbatish
Renamed parseAsmArg to conform to other function names
4dbf9af 
gcc/rust/ChangeLog:

	* expand/rust-macro-builtins-asm.cc (parseAsmArg):
	Renamed parseAsmArg to conform to other function names
	(parse_asm_arg): Likewise.
	(parse_asm): Likewise.
	* expand/rust-macro-builtins-asm.h (parseAsmArg): Likewise.
	(parse_asm_arg): Likewise.
@badumbatish
Working towards parse_reg and parse_reg_operand
347d10d 
gcc/rust/ChangeLog:

	* expand/rust-macro-builtins-asm.cc (parse_reg):
	Working towards parse_reg and parse_reg_operand
	(parse_reg_operand):
	Working towards parse_reg and parse_reg_operand
	(parse_asm_arg):
	Add todo about errors
	* expand/rust-macro-builtins-asm.h (parse_global_asm):
	remove dead code.
	(parse_nonglobal_asm):
	remove dead code.
@badumbatish
Got AST::Fragment to be created from InlineAsm
fedb5ed 
gcc/rust/ChangeLog:

	* ast/rust-expr.h (struct AnonConst):
	Got AST::Fragment to be created from InlineAsm.
	(struct InlineAsmOperand): Likewise.
	(class InlineAsm): Likewise.
	* expand/rust-macro-builtins-asm.cc (parse_reg_operand): Likewise.
	(parse_asm): likewise
CohenArthur
CohenArthur approved these changes May 30, 2024
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.

this is great progress :DD well done! a couple nitpicks and things to fix but we can merge this soon

gcc/rust/ast/rust-expr.h Outdated Show resolved Hide resolved
gcc/rust/ast/rust-expr.h Outdated Show resolved Hide resolved
gcc/rust/ast/rust-expr.h Outdated Show resolved Hide resolved
gcc/rust/ast/rust-expr.h Outdated Show resolved Hide resolved
gcc/rust/ast/rust-expr.h Outdated Show resolved Hide resolved
gcc/rust/expand/rust-macro-builtins-asm.cc Outdated
Comment on lines 222 to 241
if (!is_global_asm && check_identifier (parser, "in"))
{
return tl::nullopt;
}
else if (!is_global_asm && check_identifier (parser, "out"))
{
return tl::nullopt;
}
else if (!is_global_asm && check_identifier (parser, "lateout"))
{
return tl::nullopt;
}
else if (!is_global_asm && check_identifier (parser, "inout"))
{
return tl::nullopt;
}
else if (!is_global_asm && check_identifier (parser, "inlateout"))
{
return tl::nullopt;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this might be cleaner if it was turned into an array of strings to check for, and if any matches, return tl::nullopt. what do we check here exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this will eventually construct instances of RegisterOperand class and here i'm just setting up the scaffolding to match rust's asm.rs for easier matching right now. Different lines of code should come out from these if statements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we're checking if the identifier matches each of the name, which stands for dir_spec := "in" / "out" / "lateout" / "inout" / "inlateout" in the ABNF. i guess it also goes with whether the asm call is global or not too so we also need to check for that

gcc/rust/expand/rust-macro-builtins-asm.cc Outdated Show resolved Hide resolved
gcc/rust/expand/rust-macro-builtins-asm.cc Outdated Show resolved Hide resolved
gcc/rust/expand/rust-macro-builtins-asm.cc Outdated Show resolved Hide resolved
gcc/rust/expand/rust-macro-builtins.cc Outdated Show resolved Hide resolved
@badumbatish
Refactoring for inline asm pr
e8b9b91 
gcc/rust/ChangeLog:

	* ast/rust-expr.h (struct AnonConst):
	major refactoring of inline asm, mostly concerns
	naming convention, trinary conditionals, warnings,
	adding rust_unreachables in not-yet supported errors.
	(struct InlineAsmRegOrRegClass): Likewise.
	(struct InlineAsmOperand): Likewise.
	* expand/rust-macro-builtins-asm.cc (parse_clobber_abi): Likewise.
	(parse_reg): Likewise.
	(parse_operand): Likewise.
	(parse_reg_operand): Likewise.
	(check_and_set): Likewise.
	(parse_options): Likewise.
	(parse_format_string): Likewise.
	(parse_asm_arg): Likewise.
	(parse_asm): Likewise.
	* expand/rust-macro-builtins-asm.h (parse_asm_arg): Likewise.
	(check_identifier): Likewise.
	(check_and_set): Likewise.
	(parse_operand): Likewise.
	(parse_reg_operand): Likewise.
	(parse_options): Likewise.
	(parse_reg): Likewise.
	(parse_clobber_abi): Likewise.
	* expand/rust-macro-builtins.cc (enum class): Likewise.
	(inline_asm_maker): Likewise.
	* checks/errors/borrowck/ffi-polonius/Cargo.lock: Removed. Likewise.

gcc/testsuite/ChangeLog:
	* rust/compile/inline_asm_faulty_clobber.rs: Likewise.
	* rust/compile/inline_asm_faulty_clobber_1.rs: Likewise.
	* rust/compile/inline_asm_faulty_clobber_2.rs: Likewise.
	* rust/compile/inline_asm_illegal_options.rs: Likewise.
@badumbatish
Slim down the test cases
1aa3939 
gcc/testsuite/ChangeLog:

	* rust/compile/inline_asm_faulty_clobber.rs: compress
	the test
	* rust/compile/inline_asm_nop.rs: compress
	the test
	* rust/compile/inline_asm_faulty_clobber_1.rs: Removed.
	* rust/compile/inline_asm_faulty_clobber_2.rs: Removed.
	* rust/compile/inline_asm_nop_2.rs: Removed.
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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@badumbatish @CohenArthur