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

inline-asm: Fix some warnings #3393

Merged
merged 1 commit into from
Feb 4, 2025
Merged

inline-asm: Fix some warnings #3393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 17 additions & 7 deletions gcc/rust/expand/rust-macro-builtins-asm.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,16 +39,28 @@ std::map<AST::InlineAsmOption, std::string> InlineAsmOptionMap{
std::set<std::string> potentially_nonpromoted_keywords
= {"in", "out", "lateout", "inout", "inlateout", "const", "sym", "label"};

// Helper function strips the beginning and ending double quotes from a
// string.
std::string
strip_double_quotes (const std::string &str)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

it's probably just a nitpick on myself but back then it didnt occur to me that somebody might use this on sth like str = "'dsda'" or "abcd" despite the name being strip_double_quotes().

I think an assertion on the first and last character being double quotes would be nice :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in latest version

// Helper function strips the beginning and ending double quotes from a
// string.
std::string result = str;

rust_assert (!str.empty ());

rust_assert (str.front () == '\"');
rust_assert (str.back () == '\"');

// we have to special case empty strings which just contain a set of quotes
// so, if the string is "\"\"", just return ""
if (result.size () == 2)
return "";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this special cased, anyways?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be a good idea to use number 4 from https://en.cppreference.com/w/cpp/string/basic_string/basic_string

Copy link
Member Author

Choose a reason for hiding this comment

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

because of the assertion later on, which I did not want to tweak. since this is a static function for that one specific file I was hoping to keep it as simple and as tailored as possible - it doesn't leak anywhere else and doesn't need to be too complicated


rust_assert (result.size () >= 3);

result.erase (0, 1);
result.erase (result.size () - 1, 1);

return result;
}

Expand Down Expand Up @@ -240,12 +252,10 @@ parse_reg_operand (InlineAsmContext inline_asm_ctx)
// Loop over and execute the parsing functions, if the parser successfullly
// parses or if the parser fails to parse while it has committed to a token,
// we propogate the result.
int count = 0;
tl::expected<InlineAsmContext, InlineAsmParseError> parsing_operand (
inline_asm_ctx);
for (auto &parse_func : parse_funcs)
{
count++;
auto result = parsing_operand.and_then (parse_func);

// Per rust's asm.rs's structure
Expand Down Expand Up @@ -323,14 +333,14 @@ parse_reg_operand_in (InlineAsmContext inline_asm_ctx)
// We are sure to be failing a test here, based on asm.rs
// https://github.com/rust-lang/rust/blob/a330e49593ee890f9197727a3a558b6e6b37f843/compiler/rustc_builtin_macros/src/asm.rs#L112
rust_unreachable ();
return tl::unexpected<InlineAsmParseError> (COMMITTED);
// return tl::unexpected<InlineAsmParseError> (COMMITTED);
}

auto expr = parser.parse_expr ();

// TODO: When we've succesfully parse an expr, remember to clone_expr()
// instead of nullptr
struct AST::InlineAsmOperand::In in (reg, std::move (expr));
AST::InlineAsmOperand::In in (reg, std::move (expr));
inline_asm_ctx.inline_asm.operands.emplace_back (in, locus);
return inline_asm_ctx;
}
Expand All @@ -354,7 +364,7 @@ parse_reg_operand_out (InlineAsmContext inline_asm_ctx)

// TODO: When we've succesfully parse an expr, remember to clone_expr()
// instead of nullptr
struct AST::InlineAsmOperand::Out out (reg, false, std::move (expr));
AST::InlineAsmOperand::Out out (reg, false, std::move (expr));

inline_asm_ctx.inline_asm.operands.emplace_back (out, locus);

Expand Down
4 changes: 4 additions & 0 deletions gcc/rust/expand/rust-macro-builtins.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ const BiMap<std::string, BuiltinMacro> MacroBuiltin::builtins = {{
{"concat_idents", BuiltinMacro::ConcatIdents},
{"module_path", BuiltinMacro::ModulePath},
{"asm", BuiltinMacro::Asm},
// FIXME: Is that okay
{"llvm_asm", BuiltinMacro::Asm},
Copy link
Contributor

Choose a reason for hiding this comment

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

I remembered llvm_asm was removed last summer since the llvm_asm! is not supported anymore. Has it changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

So it has been removed in recent Rust versions, but is still used in Rust 1.49 - so we'll need to support some form of it. I think the only thing we really need to support is llvm_asm! in core, and I think we can use regular inline assembly for this

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like the only llvm_asm! usages in the rustc libraries are for black_box, which seems fairly simple, and a few stdarch things.

Copy link
Contributor

Choose a reason for hiding this comment

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

i see!

{"global_asm", BuiltinMacro::GlobalAsm},
{"log_syntax", BuiltinMacro::LogSyntax},
{"trace_macros", BuiltinMacro::TraceMacros},
Expand Down Expand Up @@ -119,6 +121,8 @@ std::unordered_map<std::string, AST::MacroTranscriberFunc>
{"format_args", format_args_maker (AST::FormatArgs::Newline::No)},
{"format_args_nl", format_args_maker (AST::FormatArgs::Newline::Yes)},
{"asm", inline_asm_maker (AST::AsmKind::Inline)},
// FIXME: Is that okay?
{"llvm_asm", inline_asm_maker (AST::AsmKind::Inline)},
{"global_asm", inline_asm_maker (AST::AsmKind::Global)},
{"option_env", MacroBuiltin::option_env_handler},
/* Unimplemented macro builtins */
Expand Down