On Sun Feb 16 18:30:32 2025 +0000, Jacek Caban wrote:
I assume it's there to detect dependency cycles. If there is a dep
cycle like `A --(import)--> B.proc2 --(forward)--> A.proc3` and we are processing the forward, then `A` would have `LDR_DONT_RESOLVE_REFS` set since module `A` hasn't completed `fixup_imports` (it's still resolving import of B.proc2). In that case adding a module dependency would result in a directed cycle `A --> B --> A`, which is unacceptable in a DAG. What does a DAG have to do with this? The dependency graph isn’t acyclic. It can’t be, since circular dependencies are allowed. We already create cycles for regular imports, so I don’t see why forward imports should be treated any differently. That said, while looking into this, I noticed that our handling of LoadCount in cyclic dependencies seemed suspicious, so I tested it. I think it's buggy. For example, if a.dll has a cyclic dependency with b.dll, and you dynamically load a.dll, take a reference to b.dll, and then unload a.dll, we would unload a.dll while keeping b.dll loaded. On Windows, neither would be unloaded until the b.dll reference is released. This isn’t strictly related to this MR, but I think it's worth mentioning.
The logic was there without comment before this patch, so I kept it
as-is after this patch. I believe it's better to have no comment than a misleading one, so if we add one, we should ensure it's accurate.
I'm not sure I understand. Are you saying that the directed graph formed by D*dag*Nodes isn't acyclic? That it is a misnomer?
Also, reference counts can only deal with acyclic (strong) references in nature. Cycles should be broken with "weak" references (e.g., adding an edge without incrementing LoadCount), or we shouldn't use "LoadCount" for dependency edges at all.