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

CentOS 7 / VCS fixes #2067

Merged
merged 12 commits into from
Mar 7, 2024
Merged

CentOS 7 / VCS fixes #2067

merged 12 commits into from
Mar 7, 2024

Conversation

GregAC
Copy link
Collaborator

@GregAC GregAC commented Aug 3, 2023

I've been getting the regression running on CentOS 7 under both VCS and Xcelium. This fixes up a mixture of CentOS 7 and VCS issues.

There will be another PR at some point with some documentation clean-ups and a specific guide for CentOS 7 as setting up the requirements needs some guidance.

Copy link
Contributor

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

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

All looks good to me. We'll need some documentation for people to add the COSIM_SIGSEGV_WORKAROUND flag, but as you say this can be done in a separate PR. In terms of the VCS fixes, were these already broken or is this a Centos 7 thing?

Comment on lines 44 to 46
'-dbname', str(md.dir_cov/'merged.vdb'),
'-report', str(md.dir_cov/'report'),
'-log', str(md.dir_cov/'merge.log'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this broken before?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, looks like it! (Or my grep-fu is not good enough :-))

@marnovandermaas
Copy link
Contributor

Also, apparently clang format is failing.

# building, riscv-fesvr isn't strictly required but the DV flow has been
# observed to see build failures where it isn't present with CentOS 7.
spike_iss_pc = ['riscv-riscv', 'riscv-disasm', 'riscv-fdt',
'riscv-fesvr']
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Maybe worth indenting to line up with the preceding [.

@@ -18,7 +18,7 @@
# As a result, passing -fno-extended-identifiers tells G++ to pretend that
# everything is ASCII, preventing strange compilation errors.
- tool: vcs
env_var: IBEX_ROOT
env_var: IBEX_ROOT, EXTRA_COSIM_CFLAGS
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need to tweak some documentation in this commit to tell (some) people to pass this flag?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should document it but as it's only really relevant to CentOS 7 and there's other things that need documenting for that also I'd prefer to deal with that in a follow up PR (in particular so we can get the various fixes in this PR in).

Comment on lines 44 to 46
'-dbname', str(md.dir_cov/'merged.vdb'),
'-report', str(md.dir_cov/'report'),
'-log', str(md.dir_cov/'merge.log'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, looks like it! (Or my grep-fu is not good enough :-))

Fixes two issues observed on CentOS 7 during testbench compile
 - pkg-config doesn't behave properly when it receives multiple flags
 - riscv-fesvr library needs to be included in build
When SpikeCosim is getting destructed a SIGSEGV was observed on CentOS
7. The root cause hasn't been identified other than it relates to the
deletion of `isa_parser_t`, potentially some kind of use after free
error.

This is an (optional) hacky workaround that simply never deletes the
`isa_parser_t` pointer in SpikeCosim. As in practise this occurs at the
end of simulation when the process is terminating the memory leak is of
little consequence.

Longer term this issue should be investigated and properly fixed.
@GregAC
Copy link
Collaborator Author

GregAC commented Feb 5, 2024

Thanks for the reviews @marnovandermaas and @rswarbrick. Once again apologies I didn't push this over the line when it was first submitted.

I've added a bunch of other little fixes mostly related to improving the flow under VCS PTAL.

Comment on lines 50 to 57
with LockedMetadata(md.dir_metadata, __file__) as md:
md.cov_merge_log = md.dir_cov / 'merge.log'
md.cov_merge_stdout = md.dir_cov / 'merge.log.stdout'
md.cov_merge_cmds = [cmd]

with open(md.cov_merge_stdout, 'wb') as fd:
logging.info("Generating merged coverage directory")
return run_one(md.verbose, cmd, redirect_stdstreams=fd)
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to keep the Metadata file open while we actually perform the coverage merge.

Suggested change
with LockedMetadata(md.dir_metadata, __file__) as md:
md.cov_merge_log = md.dir_cov / 'merge.log'
md.cov_merge_stdout = md.dir_cov / 'merge.log.stdout'
md.cov_merge_cmds = [cmd]
with open(md.cov_merge_stdout, 'wb') as fd:
logging.info("Generating merged coverage directory")
return run_one(md.verbose, cmd, redirect_stdstreams=fd)
with LockedMetadata(md.dir_metadata, __file__) as md:
md.cov_merge_log = md.dir_cov / 'merge.log'
md.cov_merge_stdout = md.dir_cov / 'merge.log.stdout'
md.cov_merge_cmds = [cmd]
with open(md.cov_merge_stdout, 'wb') as fd:
logging.info("Generating merged coverage directory")
return run_one(md.verbose, cmd, redirect_stdstreams=fd)

Additionally update metadata with the appropriate coverage merge log
information.
Programs generated from RISC-V were exiting early in configs that don't
have PMP
It's possible for a TestRunResult to contain an entry that has a path to
a build/run artifact but for that to be None rather than an actual path.
This causes the collect_results.py script to fail.

With this change such paths will be described as 'MISSING' in the
regression log instead.
An instruction stall on a FENCE.I had been mistakenly placed in an
illegal bin. A FENCE.I acts much like a branch or jump so can produce
instruction stalls just as those instructions can.
Previously some $value$plusargs calls weren't explictly specifying a
format for a number to read from the plusarg. Under some simulators this
is acceptable under others it generates an error.
Tests targetting security features make use of force to corrupt values.
Under VCS this requires building with a particular flag.
The DV flow is expecting log files to be produced with a particular file
name, without it the reporting mechanisms do not work correctly. This
adds VCS log output to a named rather rather than just capturing stdout.
@GregAC GregAC added this pull request to the merge queue Mar 7, 2024
Merged via the queue into lowRISC:master with commit eb668b0 Mar 7, 2024
11 checks passed
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.

4 participants