search

nickg / nvc

VHDL compiler and simulator
https://www.nickg.me.uk/nvc/
GNU General Public License v3.0
601 stars 75 forks source link

Old architectures are not removed upon re-analysis #710

Open Forty-Bot opened 1 year ago

Forty-Bot commented 1 year ago

Create a file like

entity test is end;
architecture test of test is begin end;

Analyze it,

nvc -a test.vhd

then rename the architecture to test2. Reanalyze and elaborate the file

nvc -a test.vhd -e test

This causes the following warning:

Warning: design unit WORK.TEST-TEST is older than its source file test.vhd and should be reanalysed

However, we just reanalyzed test.vhd! I believe the correct fix is to clean up old architectures when they are removed from their source file.

nickg commented 1 year ago

I'm not sure about unconditionally deleting the old architecture, although it probably deserves a warning at least. But I think this is actually a variant of #715: we shouldn't even be trying to load the old architecture.

Forty-Bot commented 1 year ago

The use case here is to synchronize the state of the loaded library with the source files. So the contents of the library should be exactly the same as if we always had the second architecture name. Currently, there's no way to remove old architectures without removing the library and re-analyzing everything.

I'm using --print-deps to determine which files need to be re-analyzed, but what's the point if there's no way to actually synchronize the library without a full reanalysis?

nickg commented 1 year ago

The warning should be gone with the current master.

At the moment there's no mapping from source files to the list of design units they (last) contained. We could probably add one, but it will require a bit of thought to avoid unexpected edge cases. I'll experiment a bit.

Forty-Bot commented 1 year ago

This does not completely fix the problem.

Consider instead when the architecture is in a separate file. The dependencies from --print-deps might look like

/path/to/work/_WORK.TEST.final.so: /path/to/work/WORK.TEST-TEST
/path/to/work/WORK.TEST.elab: /path/to/work/WORK.TEST-TEST
/path/to/work/WORK.TEST: test.vhd
/path/to/work/WORK.TEST-TEST: test_arch.vhd /path/to/work/WORK.TEST

Imagine that we then rename the architecture, as above. This will touch test_arch.vhd, so we know that we need to re-analyze WORK.TEST-TEST. When we do so, the new dependencies now look like

/path/to/work/_WORK.TEST.final.so: /path/to/work/WORK.TEST-TEST
/path/to/work/WORK.TEST.elab: /path/to/work/WORK.TEST-TEST
/path/to/work/WORK.TEST: test.vhd
/path/to/work/WORK.TEST-TEST2: test_arch.vhd /path/to/work/WORK.TEST
/path/to/work/WORK.TEST-TEST: test_arch.vhd /path/to/work/WORK.TEST

Now the new architecture is present, but WORK.TEST.elab won't get re-elaborated since it still depends on WORK.TEST-TEST and that file wasn't modified.

There are two ways of fixing this:

Note that --print-deps still gives the same warning as above, even with the mentioned commit. This is not as critical, since using --ignore-time with --print-deps is pretty reasonable IMO.

nickg commented 1 year ago

I'm not sure what the arch-selection algorithm is, but I think this still leaves open the possibility that test still gets selected over test2.

In 7.3.3 of the 2008 LRM it says

[..] the architecture identifier is the same as the simple name of the most recently analyzed architecture body associated with the entity declaration.

But fails to define "most recently". NVC used to do this based on the timestamp of the file in the library but now uses a sequence number which increments with each analysed architecture in the library. This seemed more robust but the implementation is quite complicated because it has to guarantee a total order even if multiple files are analysed concurrently. I'm not sure it's worth it.

Note that --print-deps still gives the same warning as above, even with the mentioned commit.

That's because --print-deps needs to load every unit in the library to see what's in it and look up its dependencies. It's the loading of the unit which triggers that warning.

Forty-Bot commented 1 year ago

But fails to define "most recently". NVC used to do this based on the timestamp of the file in the library but now uses a sequence number which increments with each analysed architecture in the library. This seemed more robust but the implementation is quite complicated because it has to guarantee a total order even if multiple files are analysed concurrently. I'm not sure it's worth it.

OK, well if that's the algorithm then there's no possibility of an incorrect selection here. I'm OK with adding a manual touch if you don't want to add removal.

That's because --print-deps needs to load every unit in the library to see what's in it and look up its dependencies. It's the loading of the unit which triggers that warning.

Yeah. But IMO that warning is not really useful, since --print-deps shouldn't care whether files are out of date or not. Whatever uses the output of --print-deps will take care of that.

nickg commented 1 year ago

OK, well if that's the algorithm then there's no possibility of an incorrect selection here. I'm OK with adding a manual touch if you don't want to add removal.

I think I'm ok with removal as long as it's not on by default. E.g. if you had to pass a --clean option to analysis which would first remove all previous units that came from that file. The problem is coming up with some way to map from a file name to the design units it last contained without having to iterate over each unit in the library and look inside. The _index file might be used for this but I want to get rid of that as it's complicated to keep up-to-date and causes various other issues (#651). That's also what prevents you being able to just directly delete design units which I agree is annoying.

Anyway I'll leave this open and investigate a bit more.