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

Variables Debugging #544

Merged
merged 24 commits into from
Sep 27, 2022
Merged

Variables Debugging #544

merged 24 commits into from
Sep 27, 2022

Conversation

ghaith
Copy link
Collaborator

@ghaith ghaith commented Aug 17, 2022

Support for debug information for variables that are accessible globally.
Solves #536

  • Added implementation for all types including structs, arrays and aliases
  • Strings are char arrays of type UTF-8 or 16
  • Structs can be nested.
    • Nested structs cannot be recursive in this version
  • Global variables are generated correctly

Not tested : POU generation

@ghaith ghaith linked an issue Aug 17, 2022 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented Aug 17, 2022

Codecov Report

Base: 93.44% // Head: 93.61% // Increases project coverage by +0.17% 🎉

Coverage data is based on head (7b8809e) compared to base (180fb07).
Patch coverage: 94.77% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #544      +/-   ##
==========================================
+ Coverage   93.44%   93.61%   +0.17%     
==========================================
  Files          42       44       +2     
  Lines       15802    16420     +618     
==========================================
+ Hits        14766    15372     +606     
- Misses       1036     1048      +12     
Impacted Files Coverage Δ
src/diagnostics.rs 82.19% <12.50%> (-0.83%) ⬇️
src/cli.rs 98.04% <71.42%> (-0.37%) ⬇️
src/lib.rs 85.80% <81.81%> (-0.16%) ⬇️
src/typesystem.rs 97.37% <91.00%> (+1.31%) ⬆️
src/datalayout.rs 96.66% <96.66%> (ø)
src/codegen/debug.rs 97.04% <97.04%> (ø)
src/ast.rs 93.45% <100.00%> (+0.04%) ⬆️
src/codegen.rs 99.20% <100.00%> (+8.52%) ⬆️
src/codegen/generators/data_type_generator.rs 93.63% <100.00%> (+0.04%) ⬆️
src/codegen/generators/expression_generator.rs 88.79% <100.00%> (ø)
... and 8 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ghaith ghaith force-pushed the debugging branch 2 times, most recently from 02c1e78 to cd27995 Compare August 31, 2022 13:49
@ghaith ghaith marked this pull request as ready for review September 7, 2022 15:23
@ghaith ghaith requested a review from riederm September 7, 2022 15:27
@ghaith
Copy link
Collaborator Author

ghaith commented Sep 7, 2022

Note this branch is still linked to the custom inkwell branch until TheDan64/inkwell#349 is merged
The commit is based on master again.


#[derive(PartialEq, Eq)]
#[allow(non_camel_case_types)]
enum DebugEncoding {
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we can find a better name for that enum - i'm not 100% what it does

what does DW_ATE stand for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

DW = Dwarf
AT = Attribute
E = Encoding, hence the name.. The parameter this is passed as is also called encoding.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The encoding here is usually only used for Basic Types, which I updated in the comment

}
}

pub struct DebugObj<'ink> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we find a better name for this? i would have assumed that the DebugObject is a small unit of debugging (like the dwarf DebugItem)

this represents all debug information of one compilation-unit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would debug builder be OK? I didin't initially use it to avoid confusion with DebugInfoBuilder (The inkwell struct)

}

/// Wraps a debug object in an enum based on its level
pub enum DebugWrapper<'ink> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

name too generic

Copy link
Collaborator

@riederm riederm left a comment

Choose a reason for hiding this comment

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

small issues, overall I like it!


fn create_array_type(
&mut self,
array: &DataTypeInformation,
Copy link
Collaborator

Choose a reason for hiding this comment

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

i would deconstruct this when calling.

you have to check for array before you call this AND you do an if-let check right at the start.
its only 3 params you're interested in


fn create_pointer_type(
&mut self,
pointer: &DataTypeInformation,
Copy link
Collaborator

Choose a reason for hiding this comment

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

deconstruct before calling

fn create_string_type(
&mut self,
name: &str,
string: &DataTypeInformation,
Copy link
Collaborator

Choose a reason for hiding this comment

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

deconstruct before calling

@@ -2348,8 +2348,8 @@ impl<'ink, 'b> ExpressionCodeGenerator<'ink, 'b> {
let target_size = self.get_string_size(left_type, right_statement.get_location())?; //we report error on parameter :-/
let value_size = self.get_string_size(right_type, right_statement.get_location())?;
let size = std::cmp::min(target_size - 1, value_size) as i64;
let align_left = left_type.get_alignment();
let align_right = right_type.get_alignment();
let align_left = left_type.get_string_inner_alignment(self.index).bytes();
Copy link
Collaborator

Choose a reason for hiding this comment

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

you had the feeling that the old name was bad ... i agree
but I think we're not talking about alignment here when considering string-encoding

i would use: bytes_per_character - and yes i know there are multi-byte characters
but i would not agree that utf16 is 2byte aligned (it would mean that a utf16 char can only start at an even byte-adress!). for me, alignment talks about the start-adress of a some data, not the inner structure.

self.get_size(index).bits()
}

pub fn get_size(&self, index: &Index) -> Offset {
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this useful to have an Offset here?

DataTypeInformation::Generic { .. } => unimplemented!("generics"),
}
}

pub fn get_alignment(&self) -> u32 {
/// Returns the String encoding's alignment (charachter)
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: charachter

@ghaith ghaith merged commit 74e42d0 into master Sep 27, 2022
@ghaith ghaith deleted the debugging branch September 27, 2022 04:48
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.

Debug - Identify/display variable values
3 participants