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

Address collected minor issues #81

Merged
merged 12 commits into from
Jul 3, 2021
Merged

Address collected minor issues #81

merged 12 commits into from
Jul 3, 2021

Conversation

kulp
Copy link
Owner

@kulp kulp commented Jan 4, 2021

During the implementation of #80, improved test regularity (for example, checking exit codes more consistently) caused some changes that do not fit within #80's scope. This pull request collects those changes and some other accumulated miscellany.

@codecov
Copy link

codecov bot commented Jan 4, 2021

Codecov Report

Merging #81 (2f17932) into develop (5e3c059) will decrease coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           develop     #81     +/-   ##
=========================================
- Coverage     96.6%   96.6%   -0.1%     
=========================================
  Files           35      35             
  Lines         2652    2651      -1     
  Branches       468     468             
=========================================
- Hits          2563    2561      -2     
  Misses          70      70             
- Partials        19      20      +1     
Impacted Files Coverage Δ
src/asmif.c 98.1% <100.0%> (-0.1%) ⬇️
src/tld.c 98.4% <100.0%> (-0.6%) ⬇️
src/tsim.c 94.9% <100.0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e3c059...2f17932. Read the comment docs.

kulp added 12 commits July 3, 2021 12:43
Failure during simulation was sometimes returning zero due to a result variable
being overridden. Bailing out upon first error is more consistent with the
existing precedent.

This required an adjustment to the computation of PERIODS.mk, since now the
number of instructions executed will not be emitted if tsim bails early (such
as when the SDL plugin is not loaded).
It does not make sense to silently lose data.
Since we look for an end-of-run instruction now, we do not need this complexity.
The longest supported filename for loading into Icarus Verilog (via simtop.v)
was 100 characters. This was demonstrated to be insufficient when working in a
`git worktree` on macOS.

    % make -s --no-print-directory -C hw/icarus -f $PWD/build/arm64-apple-darwin20.4.0/PERIODS.mk -f Makefile run_code_modification VPATH=$PWD/test/op:$PWD/test/run BUILDDIR=$PWD/build/arm64-apple-darwin20.4.0 PLUSARGS_EXTRA=+DUMPENDSTATE
    WARNING: ../verilog/ram.v:33: $readmemh: Standard inconsistency, following 1364-2005.
    ERROR: ../verilog/ram.v:33: $readmemh: Unable to open default.memh for reading.
    Could not load file private/var/folders/x5/z29gc2pj4qq9wgkcm6_lw_hc0000gn/T/tmp.lmxHT9cm/test/run/code_modification.texe
    make: *** [run_code_modification] Error 1

Note that the initial slash in the "Could not load file" is missing -- the file
name was too long by one character.

We raise the limit to 4096 characters, which is a common maximum filename
length.
@kulp kulp changed the title Collected fixes Address collected minor issues Jul 3, 2021
@kulp kulp marked this pull request as ready for review July 3, 2021 17:38
@kulp kulp merged commit 23ba5a1 into develop Jul 3, 2021
@kulp kulp deleted the collected-fixes branch July 3, 2021 17:39
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.

1 participant