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.

Sign up for GitHub

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

Lint sensitivity lists #361

Merged
merged 2 commits into from
Jan 20, 2025
+948 −21
Merged

Lint sensitivity lists #361

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e61bdb7
Initial implementation
Schottkyc137 Dec 28, 2024
af95457
refactor
Schottkyc137 Dec 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
refactor
  • Loading branch information
@Schottkyc137
Schottkyc137 committed Dec 29, 2024
commit af954575425678ed22d4e38bac3cf0e33921fb57
13 changes: 13 additions & 0 deletions vhdl_lang/src/ast/util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,19 @@ impl Name {
}
}

/// Like [self.get_suffix_reference], but disregards final indexes, such as
/// `foo.bar.baz(0)`
pub fn get_suffix_reference_disregard_index(&self) -> Option<EntityId> {
use Name::*;
match self {
Designator(suffix) => suffix.reference.get(),
Selected(_, suffix) => suffix.item.reference.get(),
Slice(name, _) => name.item.get_suffix_reference_disregard_index(),
CallOrIndexed(coi) => coi.name.item.get_suffix_reference_disregard_index(),
_ => None,
}
}

pub fn prefix(&self) -> Option<&Designator> {
match self {
Self::Attribute(attr) => attr.name.item.prefix(),
Expand Down
202 changes: 101 additions & 101 deletions vhdl_lang/src/lint/sensitivity_list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,22 +133,23 @@ fn lint_sensitivity_list(
return vec![];
}

let mut signals_in_sensitivity_list: FnvHashMap<_, _> = names
let signals_in_sensitivity_list: FnvHashMap<_, _> = names
.iter()
.flat_map(|name| {
name.item
.get_suffix_reference()
.get_suffix_reference_disregard_index()
.map(|reference| (reference, name.span))
})
.collect();
let required_sensitivity_list =
generate_sensitivity_list(root, process, signals_in_sensitivity_list.clone(), ctx);
let mut missing_signals = Vec::new();
for (eid, src_pos) in required_sensitivity_list {
if signals_in_sensitivity_list.remove(&eid).is_none() {
missing_signals.push((eid, src_pos))
}
}
let mut searcher = SensitivityListChecker {
root,
sensitivity_list: signals_in_sensitivity_list.clone(),
superfluous_entities: signals_in_sensitivity_list,
found_entities: FnvHashMap::default(),
};
let _ = process.statements.search(ctx, &mut searcher);

let mut missing_signals = searcher.found_entities.into_iter().collect_vec();
let mut diagnostics = Vec::new();
if !missing_signals.is_empty() {
missing_signals.sort_by(|(_, pos1), (_, pos2)| pos1.cmp(pos2));
Expand All @@ -160,7 +161,7 @@ fn lint_sensitivity_list(
pluralize(missing_signals.len(), "The signal", "Signals"),
missing_signals
.iter()
.map(|(sig, _)| root.get_ent(*sig).designator.to_string())
.map(|(sig, _)| format!("'{}'", root.get_ent(*sig).designator))
.join(", "),
pluralize(missing_signals.len(), "is", "are"),
),
Expand All @@ -169,13 +170,16 @@ fn lint_sensitivity_list(
for (signal, pos) in missing_signals {
diag.add_related(
pos,
format!("{} first read here", root.get_ent(signal).describe()),
format!(
"signal '{}' first read here",
root.get_ent(signal).designator
),
)
}
diagnostics.push(diag);
}

for (_, pos) in signals_in_sensitivity_list {
for (_, pos) in searcher.superfluous_entities {
diagnostics.push(Diagnostic::new(
pos.get_pos(ctx),
"Signal is never read in the process",
Expand Down Expand Up @@ -236,41 +240,18 @@ fn pluralize<'a>(len: usize, singular: &'a str, plural: &'a str) -> &'a str {
}
}

/// finds all signals that are present in a sensitivity list,
/// if the process contains the `all` keyword inside the sensitivity list.
/// This is also used to compare against the actual values of the sensitivity list
/// so that superfluous / lacking signals can be detected.
///
/// Caveats:
/// - Information from extended names is not included
/// - Information from procedure calls is not included
struct SensitivityListGenerator<'a> {
struct SensitivityListChecker<'a> {
root: &'a DesignRoot,
/// The sensitivity list present at the generate statement
sensitivity_list: FnvHashMap<EntityId, TokenSpan>,
/// Additional entities that are in the Sensitivity list, but are never read in the process.
superfluous_entities: FnvHashMap<EntityId, TokenSpan>,
/// A set of named entities that form the sensitivity list.
/// Additionally, the token span of the first occurrence is also provided.
found_entities: FnvHashMap<EntityId, SrcPos>,
}

/// Generate a sensitivity list from a process statement
fn generate_sensitivity_list(
root: &DesignRoot,
process: &ProcessStatement,
sensitivity_list: FnvHashMap<EntityId, TokenSpan>,
ctx: &dyn TokenAccess,
) -> FnvHashMap<EntityId, SrcPos> {
let mut searcher = SensitivityListGenerator {
root,
sensitivity_list,
found_entities: FnvHashMap::default(),
};
let _ = process.statements.search(ctx, &mut searcher);

searcher.found_entities
}

impl SensitivityListGenerator<'_> {
impl SensitivityListChecker<'_> {
fn analyze_expression(&mut self, expr: &Expression, span: TokenSpan, ctx: &dyn TokenAccess) {
match expr {
Expression::Binary(_, lhs, rhs) => {
Expand All @@ -295,7 +276,7 @@ impl SensitivityListGenerator<'_> {
Expression::Qualified(qual_expr) => {
self.analyze_expression(&qual_expr.expr.item, qual_expr.expr.span, ctx)
}
Expression::Name(name) => self.analyze_name(name, span, ctx),
Expression::Name(name) => self.analyze_name(name, span, ctx, false),
Expression::New(allocator) => match &allocator.item {
Allocator::Qualified(qual_expr) => {
self.analyze_expression(&qual_expr.expr.item, qual_expr.expr.span, ctx)
Expand All @@ -307,21 +288,27 @@ impl SensitivityListGenerator<'_> {
}
}

fn analyze_name(&mut self, name: &Name, pos: TokenSpan, ctx: &dyn TokenAccess) {
fn analyze_name(
&mut self,
name: &Name,
pos: TokenSpan,
ctx: &dyn TokenAccess,
remove_only: bool,
) {
use Name::*;
match name {
Designator(designator) => {
self.analyze_designator(designator, pos, ctx);
self.analyze_designator(designator, pos, ctx, remove_only);
}
Selected(_, suffix) => {
// self.analyze_name(&prefix.item, prefix.span, ctx);
self.analyze_designator(&suffix.item, pos, ctx);
Selected(prefix, suffix) => {
self.analyze_name(&prefix.item, prefix.span, ctx, true);
self.analyze_designator(&suffix.item, pos, ctx, remove_only);
}
SelectedAll(prefix) => self.analyze_name(&prefix.item, prefix.span, ctx),
Slice(prefix, _) => self.analyze_name(&prefix.item, prefix.span, ctx),
SelectedAll(prefix) => self.analyze_name(&prefix.item, prefix.span, ctx, remove_only),
Slice(prefix, _) => self.analyze_name(&prefix.item, prefix.span, ctx, remove_only),
Attribute(attr) => self.analyze_attribute_name(attr, ctx),
CallOrIndexed(coi) => {
self.analyze_name(&coi.name.item, coi.name.span, ctx);
self.analyze_name(&coi.name.item, coi.name.span, ctx, remove_only);
for item in &coi.parameters.items {
match &item.actual.item {
ActualPart::Expression(expr) => {
Expand All @@ -342,17 +329,29 @@ impl SensitivityListGenerator<'_> {
designator: &WithRef<Designator>,
pos: TokenSpan,
ctx: &dyn TokenAccess,
remove_only: bool,
) {
if let Some(reference) = designator.reference.get() {
self.superfluous_entities.remove(&reference);
let ent = self.root.get_ent(reference);
if ent.is_signal() {
self.sensitivity_list.remove(&ent.id);
if ent.is_signal() && !self.sensitivity_list.contains_key(&reference) && !remove_only {
self.found_entities
.entry(reference)
.or_insert_with(|| pos.get_pos(ctx));
}
}
}

fn analyze_attribute_name(&mut self, attr: &AttributeName, ctx: &dyn TokenAccess) {
self.analyze_name(&attr.name.item, attr.name.span, ctx);
use AttributeDesignator::*;
match attr.attr.item {
Ident(_) | Image => {}
// These likely do not belong inside a sensitivity list.
// However, true analysis can only be performed when it is known whether the name is
// static or not.
_ => return,
}
self.analyze_name(&attr.name.item, attr.name.span, ctx, false);
if let Some(expr) = &attr.expr {
self.analyze_expression(&expr.item, expr.span, ctx)
}
Expand Down Expand Up @@ -411,7 +410,7 @@ impl SensitivityListGenerator<'_> {
fn analyze_discrete_range(&mut self, range: &DiscreteRange, ctx: &dyn TokenAccess) {
match range {
DiscreteRange::Discrete(name, range) => {
self.analyze_name(&name.item, name.span, ctx);
self.analyze_name(&name.item, name.span, ctx, false);
if let Some(range) = range {
self.analyze_range(range, ctx)
}
Expand Down Expand Up @@ -460,7 +459,7 @@ impl SensitivityListGenerator<'_> {
}
}

impl Searcher for SensitivityListGenerator<'_> {
impl Searcher for SensitivityListChecker<'_> {
fn search_decl(&mut self, ctx: &dyn TokenAccess, decl: FoundDeclaration<'_>) -> SearchState {
use SequentialStatement::*;
if let DeclarationItem::SequentialStatement(stmt) = &decl.ast {
Expand Down Expand Up @@ -546,29 +545,16 @@ enum ProcessCategory {
/// ```
/// but won't work for some more exotic, mixed processes.
fn get_likely_process_category(root: &DesignRoot, process: &ProcessStatement) -> ProcessCategory {
// This might be incorrect for weird mixed processes like
// ```
// process (clk) is
// begin
// if rising_edge(clk) then
// // ...
// end if;
// a <= b or c;
// end process;
// ```
if process.statements.len() > 1 {
return ProcessCategory::Combinational;
};
// using iter().all(...) guards against cases like the following:
// using iter().any(...) guards against edge cases like the following:
// ```
// process(clkA, clkB) is
// begin
// if rising_edge(clkA) then
// end if;
// if rising_edge(clkB) then
// end if;
//
// some_assignment <= xy;
// end process;
let is_likely_clocked = process.statements.iter().all(|stmt| {
let is_likely_clocked = process.statements.iter().any(|stmt| {
match &stmt.statement.item {
SequentialStatement::If(if_stmt) => {
// this is always guaranteed to be present
Expand Down Expand Up @@ -630,23 +616,14 @@ fn is_likely_clocked(root: &DesignRoot, expression: &Expression) -> bool {
mod tests {
use super::*;
use crate::analysis::tests::{check_no_diagnostics, LibraryBuilder};
use crate::syntax::test::Code;
use crate::syntax::test::check_diagnostics;
use std::cell::Cell;

fn get_ent(root: &DesignRoot, code: Code) -> (EntityId, SrcPos) {
(
root.search_reference(code.source(), code.start())
.unwrap()
.id,
code.pos(),
)
}

#[test]
fn extract_sensitivity_list_no_items() {
let mut builder = LibraryBuilder::new();

let _ = builder.code(
builder.code(
"libname",
"
entity ent is
Expand All @@ -662,11 +639,12 @@ end architecture;",

let (root, diagnostics) = builder.get_analyzed_root();
check_no_diagnostics(&diagnostics);

let num_of_searches = Cell::new(0);
let mut searcher = ProcessSearcher::new(|proc, ctx| {
num_of_searches.set(num_of_searches.get() + 1);
let res = generate_sensitivity_list(&root, proc, ctx);
assert_eq!(res, FnvHashMap::default());
let diag = lint_sensitivity_list(&root, ctx, proc);
assert_eq!(diag, Vec::default());
});
let _ = root.search(&mut searcher);
assert_eq!(num_of_searches.get(), 1)
Expand All @@ -688,15 +666,33 @@ end entity;
architecture a of ent is
-- all of these signals should be considered as being 'read' in the sensitivity list
signal sig_b, sig_c, sig_d, sig_e, sig_f, sig_g, sig_h, sig_i, sig_j, sig_k, sig_l : bit;
-- all of these signals are fine and present in the sensitivity list
signal sig_0 : bit;
signal sig_1 : bit_vector(1 downto 0);
type t_rec is record
foo : bit;
end record;
signal sig_2 : t_rec;
signal sig_3, sig_4, sig_5, sig_6 : bit;
-- superfluous signal
signal additional : bit;
signal void : bit;

function func(din: bit) return bit is
begin
end func;
begin
process is
process(sig_0, sig_1(0), sig_2, sig_3, sig_4, sig_5, sig_6, additional) is
variable void_var : bit;
begin
if (sig_5 = '1') then
void <= sig_6;
end if;
void <= sig_3 or sig_4;
void <= sig_3;
void <= sig_2.foo;
void <= sig_0;
void <= sig_1(0);
void <= sig_a;
void <= sig_b;
void <= sig_b;
Expand All @@ -720,25 +716,29 @@ end architecture;",
check_no_diagnostics(&diagnostics);

let expected_signals = [
get_ent(&root, code.s("sig_a", 2)),
get_ent(&root, code.s("sig_b", 2)),
get_ent(&root, code.s("sig_c", 2)),
get_ent(&root, code.s("sig_d", 2)),
get_ent(&root, code.s("sig_e", 2)),
get_ent(&root, code.s("sig_f", 2)),
get_ent(&root, code.s("sig_g", 2)),
get_ent(&root, code.s("sig_h", 2)),
get_ent(&root, code.s("sig_i", 2)),
get_ent(&root, code.s("sig_j", 2)),
get_ent(&root, code.s("sig_k", 2)),
get_ent(&root, code.s("sig_l", 2)),
];
"sig_a", "sig_b", "sig_c", "sig_d", "sig_e", "sig_f", "sig_g", "sig_h", "sig_i",
"sig_j", "sig_k", "sig_l",
]
.map(|value| (value, code.s(value, 2).pos()));

let num_of_searches = Cell::new(0);
let mut searcher = ProcessSearcher::new(|proc, ctx| {
num_of_searches.set(num_of_searches.get() + 1);
let res = generate_sensitivity_list(&root, proc, ctx);
assert_eq!(res, expected_signals.iter().cloned().collect());
let res = lint_sensitivity_list(&root, ctx, proc);
let mut expected_missing_diag = Diagnostic::new(
code.s1("process").pos(),
"Signals 'sig_a', 'sig_b', 'sig_c', 'sig_d', 'sig_e', 'sig_f', 'sig_g', 'sig_h', 'sig_i', 'sig_j', 'sig_k', 'sig_l' are not read in the sensitivity list",
ErrorCode::MissingInSensitivityList
);
for (name, pos) in &expected_signals {
expected_missing_diag.add_related(pos, format!("signal '{name}' first read here"));
}
let expected_superfluous_diag = Diagnostic::new(
code.s("additional", 2),
"Signal is never read in the process",
ErrorCode::SuperfluousInSensitivityList,
);
check_diagnostics(res, vec![expected_missing_diag, expected_superfluous_diag]);
});
let _ = root.search(&mut searcher);
assert_eq!(num_of_searches.get(), 1)
Expand Down Expand Up @@ -845,6 +845,6 @@ end architecture;",
idx.set(idx.get() + 1);
});
let _ = root.search(&mut searcher);
assert_eq!(idx.get(), 8);
assert_eq!(idx.get(), 9);
}
}
Loading