[lld] LLD's software architecture (update)

Hi,

I’ve been following the changes in LLD’s software architecture.

Recently a new file was added: Alias.h

In this file, the curret Atom set is extended with an AliasAtom.

While the change seems innocent enough, it has some nasty
potential. Simple.h and Alias.h are placed in the ReaderWriter
component. This is the 2nd component in the layering:

Driver
ReaderWriter

Passes
Core

The problem is that Core depends on these files. So in my opinion,
they belong to that Component. The greater impact here is that
once core starts depending on ReaderWriter, architecture Erosion
will happen. This would obviously not be a good thing, since the
current design is fresh and clean.

I would love to supply patches, but am having issues getting
lld to build under Ubuntu 12.04. (but that’s another story :wink: ).

I’ve posted the updated architecture view here:
http://www.c2lang.org/docs/lld_architecture_20140602.png

Hi,

I've been following the changes in LLD's software architecture.
Recently a new file was added: Alias.h
In this file, the curret Atom set is extended with an AliasAtom.
While the change seems innocent enough, it has some nasty
potential. Simple.h and Alias.h are placed in the ReaderWriter
component. This is the 2nd component in the layering:
Driver
ReaderWriter
Passes
Core

The problem is that Core depends on these files. So in my opinion,
they belong to that Component. The greater impact here is that
once core starts depending on ReaderWriter, architecture Erosion
will happen. This would obviously not be a good thing, since the
current design is fresh and clean.

Core's dependency to Simple.h was there before the addition of Alias.h, so
it's not new. And I don't feel like it's that bad as you might be feeling.
Do you have any suggestion to get rid of it? Just moving the file to Core?

I would love to supply patches, but am having issues getting

The inverted dependency of Core to ReaderWriter via Simple.h was already present.

My idea was to fix the problem before it gets bigger.

My proposal would be to move Simple.h and Alias.h to Core. Similar to

UndefinedAtom.h etc.

It would be even nicer to make the naming consistent as well, since there already is

UndefinedAtom.h

SharedLibaryAtom.h
etc

Maybe:
SimpleAtom.h

AliasAtom.h

Moving the file to core fixes the dependencies.

I agree to move these files to Core. Any objections?

I agree to move these files to Core. Any objections?

None here.

- Michael Spencer

I will create a patch to move the files and send it for review.

I’ve run a new scan of LLD’s architecture and it appears to be getting cleaner!

There are still some upward dependencies however:

  • core depends on Reader and Writer

  • passes depend on Reader and Writer

  • ReaderWriter/PECOFF/ReaderCOFF.cpp depends on Driver.h

The updated architecture can be seen here:

http://www.c2lang.org/docs/lld_architecture_20140710.png

Hi

0001-Remove-LayoutPass-dependency-on-ReaderWriter.patch (680 Bytes)

Well, it does have a Registry& that’s defined in Reader.h.

But that can be forward declared (and apparently is in some
of the other headers it includes.

Using forward-declaration would not fix the layer violation.

Yes, but if you look in LayoutPasses.c, it calls registry.referenceKindToString(…),
so it does need the definition…

Not so easy… :slight_smile:

Trade a slightly worse error message for the removal of a layering violation.

0001-Remove-LayoutPass-dependency-on-ReaderWriter.patch (684 Bytes)

0002-Remove-layering-violation-by-accepting-a-slightly-wo.patch (23.1 KB)

Trade a slightly worse error message

... which isn't used in Release Builds anyway, I might add. :slight_smile:

aw