-
Notifications
You must be signed in to change notification settings - Fork 46
Fix evaluation of non-dot assignments containing forward references #510
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.
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
base: main
Are you sure you want to change the base?
Conversation
e6be6a3 to
56ebad4
Compare
quic-seaswara
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Can we define a hierarchy of what assignments are evaluated in order, Yes it makes sense to evaluate in the order that the scripts define them, but users would not know the assignment order. I need this to be documented thoroughly.
- Please also add assignment id's similar to rule id's that we can annotate to let the user know which assignment was picked up when evaluating a symbol.
These two patches can be a pre requisite before bringing this patch in.
Few questions before I review this change in more detail
- What happens with PROVIDE assignments ?
- An example illustration is symdef fiiles, defsym assignments and fixing the order would be very crucial.
- ld.eld --defsym foo=1 -T script.t --defsym bar=2 (what is the category of symbol assignments assigned to foo=1 and bar=2)
- ld.eld --defsym foo=1 obj.symdef -T script.t --defsym bar=2 (what is the category of symbol assignments assigned to foo=1 and bar=2) when there is a symdef file
- section magic symbols and assigning values to it
- How is garbage collection affected by what symbols are evaluated especially with PROVIDE and non PROVIDE use cases
| DIAG(verbose_performing_layout_iteration, DiagnosticEngine::Verbose, | ||
| "Performing layout iteration %0") | ||
| DIAG(verbose_eval_pending_assignments, DiagnosticEngine::Verbose, | ||
| "Evaluating pending assignments") No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a %0 with the assignment expression
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot add assignment expression here. This diagnostic marks the beginning of pending assignment evaluations. We do not report this diagnostic per assignment.
| DIAG(verbose_infer_target, DiagnosticEngine::Verbose, | ||
| "Inferred target : %0") | ||
| DIAG(verbose_performing_layout_iteration, DiagnosticEngine::Verbose, | ||
| "Performing layout iteration %0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we --trace=layout and add the layout diagnostics in tracing layout iterations ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this sounds good to me. I think a lot of things can be covered under --trace=layout. Can we handle this as a separate task altogether that analysis what all diagnostics should fall under --trace=layout?
|
|
||
| bool m_NeedPhdr = false; | ||
|
|
||
| std::unordered_set<const ResolveInfo *> PartiallyEvaluatedSymbols; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be PartiallyEvaluatedAssignments instead ? because everything is an assignment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need both PartiallyEvaluatedAssignments and PartiallyEvaluatedSymbols. With just storing assignments, we cannot determine if a symbol was partially evaluated or not, as we do not maintain a mapping of symbol nodes to assignments.
|
|
||
| // If symbol originates from a linker script, record it in NamePool. | ||
| if (Input && Input->isLinkerScript()) | ||
| ThisModule.getNamePool().addScriptSymbol(Result.Info); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about defsym ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defsym is handled in the exact same way as other linker script assignments.
| } | ||
|
|
||
| if (Input && Input->isLinkerScript()) | ||
| ThisModule.getNamePool().addScriptSymbol(OutputSym->resolveInfo()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why in two places ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One is used by non-PROVIDE linker script assignments, and the other is used by PROVIDE linker script assignments.
|
@quic-seaswara Thank you for the detailed review.
I have added documentation detailing linker script symbol assignment evaluation order in #597.
I am not sure if we should make it this complicated. It is either the assignment that was before the symbol reference, or the final assignment for the symbol. Those are the only two valid assignments for which the symbol value can be taken from. Can we please prioritize this analysis feature later as per the requirements?
The framework that determines if
I have added a note for
Currently, we do not create an assignment node for symdef symbol assignments. So, they do not affect assignment evaluation order.
Can you please explain more about this concern?
I think this falls outside the scope of this PR functionality. This PR only affects forward-reference evaluation in the layout step. Garbage-collection happens much before. |
56ebad4 to
1006bc5
Compare
1006bc5 to
fe0bd0a
Compare
fe0bd0a to
b4c13f1
Compare
b4c13f1 to
56498ff
Compare
This commit fixes evaluation of non-dot assignments containing forward
references. At a high-level, eld evaluates assignments in sequence
and consequently the expressions containing forward references needs to
be reevaluated when the forward reference value is computed for the correct
computation results. For example:
```
u = v1; // Assignment 1
v1 = v2; // Assignment 2
v2 = 0xa; // Assignment 3
```
eld evaluates the assignments in order, that is, the evaluation happens
as: [Assignment 1, Assignment 2, Assignment 3]. If we follow this
evaluation order, the symbols `u` and `v1` will have incorrect values
because the value of `v2` is unknown when the assignments of `u` and
`v1` are evaluated.
This commit fixes evaluation of non-dot assignments containing forward
references by making the below two changes:
1) Assignments which cannot be completely evaluated during the
sequential evaluation of expressions are marked as pending
assignments. After the layout is done, but before the relaxation,
these pending assignments are recursively reevaluated until there
is no improvement in pending assignments or a MaxIteration limit is
reached.
2) During a layout iteration, assignments may get evaluated multiple times
as layout needs to reset itself based on a few conditions
(new segment needs to be created, and so on...). It is important to reset
the symbol if a layout gets reset. Let's
see why it is important with the help of an example:
```
SECTIONS {
FOO : {
u = v; // Assignment 1
v = 0xa; // Assignment 2
*(.text.foo)
}
BAR : {
v = 0xb; // Assignment 3
}
v = 0xc; // Assignment 4
}
```
The sequential assignment evaluation order is: [A1, A2, A3, A4]. When
A1 is evaluated, `v` is not defined, hence we mark A1 as pending
assignment. However, if the layout gets reset after evaluating A2,
then A1 will be evaluated again, but this time, `v` is defined (from
assignment 2 evaluation) and thus A1 can be completely evaluated.
This is wrong because A1 should get the value from the assignment 4
instead of assignment 2!
We fix this issue by resetting the symbol values
whenever a layout is reset.
The same issue happens when the layout needs to be recomputed after a
relaxation pass. And the same solution of resetting the
the symbol values works for this case as well.
This commit also adds a new trace category 'pending-assignments' for
tracing pending assignment evaluation.
Resolves qualcomm#505
Signed-off-by: Parth Arora <partaror@qti.qualcomm.com>
56498ff to
af10850
Compare
This commit fixes evaluation of non-dot assignments containing forward references. At a high-level, eld evaluates assignments in sequence and consequently the expressions containing forward references needs to be reevaluated when the forward reference value is computed for the correct computation results. For example:
eld evaluates the assignments in order, that is, the evaluation happens as: [Assignment 1, Assignment 2, Assignment 3]. If we follow this evaluation order, the symbols
uandv1will have incorrect values because the value ofv2is unknown when the assignments ofuandv1are evaluated.This commit fixes evaluation of non-dot assignments containing forward references by making the below two changes:
Assignments which cannot be completely evaluated during the
sequential evaluation of expressions are marked as pending
assignments. After the layout is done, but before the relaxation,
these pending assignments are recursively reevaluated until there
is no improvement in pending assignments or a MaxIteration limit is
reached.
During a layout iteration, assignments may get evaluated multiple times
as layout needs to reset itself based on a few conditions
(new segment needs to be created, and so on...). It is important to reset
the symbol if a layout gets reset. Let's
see why it is important with the help of an example:
The sequential assignment evaluation order is: [A1, A2, A3, A4]. When A1 is evaluated,
vis not defined, hence we mark A1 as pending assignment. However, if the layout gets reset after evaluating A2, then A1 will be evaluated again, but this time,vis defined (from assignment 2 evaluation) and thus A1 can be completely evaluated. This is wrong because A1 should get the value from the assignment 4 instead of assignment 2!We fix this issue by resetting the symbol values
whenever a layout is reset.
The same issue happens when the layout needs to be recomputed after a relaxation pass. And the same solution of resetting the the symbol values works for this case as well.
This commit also adds a new trace category 'pending-assignments' for tracing pending assignment evaluation.
Resolves #505