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.

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

program: remove references from ProgramSpec #679

Merged
merged 1 commit into from
Jun 2, 2022

Conversation

lmb
Copy link
Collaborator

@lmb lmb commented May 19, 2022

Now that we don't need ProgramSpec.layout anymore to reconstruct
CO-RE relocations we can remove ProgramSpec.references. At the
same time we can simplify the linking logic.

@lmb lmb requested a review from ti-mo May 19, 2022 19:04
Copy link
Collaborator

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

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

You've revived the old linking code in the form of flattenInstructions and I still find it hard to wrap my head around coming back to it a few months later.

Could we maybe go at this another way and introduce a step that resolves stringly prog references to *ProgramSpecs in insn metadata? We could support either a string or *ProgramSpec in the same meta field. This would mean we no longer need to pass refs around, which will make the linking code a lot easier to understand.

linker.go Outdated Show resolved Hide resolved
@lmb
Copy link
Collaborator Author

lmb commented May 25, 2022

I still find it hard to wrap my head around coming back to it a few months later.

Can you be more specific, what part of it is hard to understand? From the rest of you comment it seems refs is kind of ambiguous?

Could we maybe go at this another way and introduce a step that resolves stringly prog references to *ProgramSpecs in insn metadata?

I think it only makes sense to use metadata if we need to pass info from the ELF loader to NewProgram or similar. So if the only use case is in the ELF loader we should come up with a different solution.

Now that we don't need ProgramSpec.layout anymore to reconstruct
CO-RE relocations we can remove ProgramSpec.references. At the
same time we can simplify the linking logic.
@lmb lmb force-pushed the prog-remove-references branch from b3b0784 to 57134c1 Compare May 27, 2022 13:01
linker.go Show resolved Hide resolved
@ti-mo ti-mo merged commit 6294837 into cilium:master Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants