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

backend: make dynlib handling target-agnostic #796

Merged
merged 7 commits into from
Jul 16, 2023

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Jul 13, 2023

Summary

Make code generation for .dynlib procedure/variable setup target-
agnostic and move it to the unified backend processing pipeline.
Instead of as part of the data-init module operator, loading dynamic
libraries and setting up the procedures and variables now happens in the
the new dynlib-init module operator (which is called directly after
the data-init operator).

This fixes run-time expressions in .dynlib pragmas not being subject
to destructor injection and removes the last PNode side-channel
(i.e., AST reaching the code generators through something else than
genProc).

Details

The C code generator previously created globals for holding the handle
of loaded libraries in an ad-hoc way, with the PLib associated with
the symbol modified to store the generated C name.

This same approach is not possible with the unified pipeline, and
instead, more of the TLib related handling is moved into semantic
analysis:

  • remove the TLib.generated field
  • store a symbol instead of a raw name in TLib.name
  • the symbol for each PLib of a module is generated when the module
    is closed
  • adjust PackedLib and the related load/store logic

With this, a TLib is no longer modified outside of semantic analysis,
and the code generators don't need to introduce ad-hoc globals.

The queueing logic in process is adjusted to also consider imported
.dynlib procedures, with them now having dedicated processing. When
they're processed, the procedures are announced to process's caller
and the loader logic is generated and emitted. The generated loader
logic is, apart from being generated as MIR code, stays as it was.

One problem is with dynlib variables. How they're represented is left to
the code generators, which means that a normal assignment cannot be used
to initialize the internal representation. For this reason, a new
internal magic (mAsgnDynlibVar) that is used for setting up a dynlib
procedures/variables is introduced and implemented in cgen.

Finally, the workarounds regarding dynlib procedures are removed from
cbackend and backends and the everything dynlib-init specific is
removed from cgen. Reporting dependencies on statically-known dynamic
libraries is now handled in cbackend.

While now theoretically possible, the JS and VM backend still don't
support dynlib procedure and variables.

Summary
=======

Make code generation for `.dynlib` procedure/variable setup target-
agnostic and move it to the unified backend processing pipeline.
Instead of as part of the *data-init* module operator, loading dynamic
libraries and setting up the procedures and variables now happens in the
the new *dynlib-init* module operator (which is called directly after
the *data-init* operator).

This fixes run-time expressions in `.dynlib` pragmas not being subject
to destructor injection and removes the last `PNode` side-channel
(i.e., AST reaching the code generators through something else than
`genProc`).

Details
=======

The C code generator previously created globals for holding the handle
of loaded libraries in an ad-hoc way, with the `PLib` associated with
the symbol modified to store the generated C name.

This same approach is not possible with the unified pipeline, and
instead, more of the `TLib` related handling is moved into semantic
analysis:
- remove the `TLib.generated` field
- store a symbol instead of a raw name in `TLib.name`
- the symbol for each `PLib` of a module is generated when the module
  is closed
- adjust `PackedLib` and the related load/store logic

With this, a `TLib` is no longer modified outside of semantic analysis,
and the code generators don't need to introduce ad-hoc globals.

The queueing logic in `process` is adjusted to also consider imported
`.dynlib` procedures, with them now having dedicated processing. When
they're processed, the procedures are announced to `process`'s caller
and the loader logic is generated and emitted. The generated loader
logic is, apart from being generated as MIR code, stays as it was.

One problem is with dynlib variables. How they're represented is left to
the code generators, which means that a normal assignment cannot be used
to initialize the internal representation. For this reason, a new
internal magic (`mAsgnDynlibVar`) that is used for setting up a dynlib
procedures/variables is introduced and implemented in `cgen`.

Finally, the workarounds regarding dynlib procedures are removed from
`cbackend` and `backends` and the everything dynlib-init specific is
removed from `cgen`. Reporting dependencies on statically-known dynamic
libraries is now handled in `cbackend`.

While now theoretically possible, the JS and VM backend still don't
support dynlib procedure and variables.
@zerbina zerbina added bug Something isn't working refactor Implementation refactor compiler/sem Related to semantic-analysis system of the compiler compiler/backend Related to backend system of the compiler labels Jul 13, 2023
@zerbina zerbina added this to the C backend refactoring milestone Jul 13, 2023
Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

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

Looks good, and I really like how it generalized the dynamic library facility. I did leave a soft suggestion.

On a sidenote, I noticed I struggled remembering what forward did from the mir DSL. It could well just be me, but I'll also consider a better name.

compiler/backend/cbackend.nim Show resolved Hide resolved
@zerbina
Copy link
Collaborator Author

zerbina commented Jul 15, 2023

That should fix the IC test failures. Previously, without #797, the symbols, and thus the their associated TLib, were stored into the packed representation before the globals for the TLib instance was created, meaning that all TLib instances had 'nil' names when loading the rod files.

I also fixed a small oversight of mine where a global was also created for TLib instances that represent .header information.

On a sidenote, I noticed I struggled remembering what forward did from the mir DSL. It could well just be me, but I'll also consider a better name.

Hm, yeah, I went through a lot of back and forth with the name, but never really arrived at something self-explanatory (which could indicate that the facility itself is the problem).

forward is for chains that don't end in a sink node because the sink construct cannot be represented in the DSL (an if, for example). While the template simply discards the EValue internally, it is meant to signal to the person reading the code that the MIR code representing the value's consumer is (and needs to be) emitted next.

On non-Windows systems, an extra message is echoed on library loading
failure.
@zerbina zerbina marked this pull request as ready for review July 16, 2023 16:59
@zerbina
Copy link
Collaborator Author

zerbina commented Jul 16, 2023

/merge

@github-actions
Copy link

Merge requested by: @zerbina

Contents after the first section break of the PR description has been removed and preserved below:


Notes for Reviewers

@chore-runner chore-runner bot added this pull request to the merge queue Jul 16, 2023
Merged via the queue into nim-works:devel with commit aabe9eb Jul 16, 2023
27 checks passed
@zerbina zerbina deleted the target-agnostic-dynlib-handling branch July 16, 2023 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler/backend Related to backend system of the compiler compiler/sem Related to semantic-analysis system of the compiler refactor Implementation refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants