feat: Add ASSERT command parsing support to linker scripts#1607
Conversation
|
The AArch64 test appears to be failing during the wild-action installation step (exit code 2), not during actual test execution. All other checks pass and tests pass locally. Let me know if I should investigate further or if this is a known CI issue. |
davidlattimore
left a comment
There was a problem hiding this comment.
Thanks for working on this :)
Update: Expression Parsing ImplementedI've implemented full expression parsing as requested. The parser now handles: Supported features:
Tests added:
Scope documentation: |
davidlattimore
left a comment
There was a problem hiding this comment.
Sorry about the slow review. Looks like I started reviewing and had some draft comments, but forgot to send them and I only just noticed.
|
|
||
| /// Parse comparison operators: <, >, <=, >=, ==, != | ||
| fn parse_comparison<'a>(input: &mut &'a BStr) -> winnow::Result<Expression<'a>> { | ||
| let mut left = parse_additive.parse_next(input)?; |
There was a problem hiding this comment.
I'd expect that the left hand side of a comparison would itself be an expression
|
|
||
| /// Parse an expression - entry point for expression parsing | ||
| fn parse_expression<'a>(input: &mut &'a BStr) -> winnow::Result<Expression<'a>> { | ||
| parse_comparison.parse_next(input) |
There was a problem hiding this comment.
I know for an assertion that an expression must be a comparison, but not all expressions need to be comparisons. I guess there are two options. You could change parse_assert to call parse_comparison, or you could parse an arbitrary expression, and then at some point we validate that that expression is indeed a comparison.
There was a problem hiding this comment.
In gnu ld it treats any expression that resolve to non zero value as valid in assertion and if it reolves to zero it stops. So do you recommend i should check for comparison later after parsing the expression or i follow the same behaviour as gnu ld
There was a problem hiding this comment.
Yes, I'd say that type checking should be a separate pass and that we shouldn't try to bake type checking into parsing.
|
|
||
| alt(( | ||
| // Parentheses - parse expression inside | ||
| delimited('(', parse_comparison, ')'), |
There was a problem hiding this comment.
Why is a comparison what we'd expect inside parenthesis? What about other expressions like a * (b - c)? Would a comparison as anything but the top-level be valid?
There was a problem hiding this comment.
Will change from parse_comparison to parse_expression
|
Thanks for reviewing will take my time to read more about this and resolve the issues. |
… chaining - Changed while to if in parse_comparison to prevent chaining (a < b < c) - Use parse_expression in parentheses, ALIGN, MIN, MAX for clarity - Separates parsing from validation per maintainer feedback
Adds parsing support for
ASSERT(expression, "message")commands in linker scripts. Both top-level and withinSECTIONSblocks are supported.Assertions are parsed and stored in the AST but not yet evaluated. This is commonly used in Linux kernel linker scripts to validate memory layout constraints.
Changes
Assertvariant toCommandandSectionCommandenumsparse_assert()functionlayout_rules.rs(currently no-op with TODO)Evaluation will be implemented in a follow-up PR.