-
Notifications
You must be signed in to change notification settings - Fork 25
Provide unique name from closures with same name #488
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,6 +1,7 @@ | ||||||
| //! The LLVM IR generator context. | ||||||
|
|
||||||
| use std::cell::RefCell; | ||||||
| use std::collections::BTreeMap; | ||||||
| use std::collections::HashMap; | ||||||
| use std::rc::Rc; | ||||||
|
|
||||||
|
|
@@ -84,6 +85,10 @@ pub struct Context<'ctx> { | |||||
| current_function: Option<Rc<RefCell<Function<'ctx>>>>, | ||||||
| /// The loop context stack. | ||||||
| loop_stack: Vec<Loop<'ctx>>, | ||||||
| /// Monotonic counter for unique frontend function name mangling. | ||||||
| function_counter: usize, | ||||||
| /// Scope stack mapping Yul names to mangled LLVM names. | ||||||
| function_scope: Vec<BTreeMap<String, String>>, | ||||||
| /// The PVM memory configuration. | ||||||
| memory_config: SolcStandardJsonInputSettingsPolkaVMMemory, | ||||||
|
|
||||||
|
|
@@ -246,6 +251,8 @@ impl<'ctx> Context<'ctx> { | |||||
| functions: HashMap::with_capacity(Self::FUNCTIONS_HASHMAP_INITIAL_CAPACITY), | ||||||
| current_function: None, | ||||||
| loop_stack: Vec::with_capacity(Self::LOOP_STACK_INITIAL_CAPACITY), | ||||||
| function_counter: 0, | ||||||
| function_scope: Vec::new(), | ||||||
| memory_config, | ||||||
|
|
||||||
| debug_info, | ||||||
|
|
@@ -456,12 +463,23 @@ impl<'ctx> Context<'ctx> { | |||||
| location: Option<(u32, u32)>, | ||||||
| is_frontend: bool, | ||||||
| ) -> anyhow::Result<Rc<RefCell<Function<'ctx>>>> { | ||||||
| assert!( | ||||||
| self.get_function(name, is_frontend).is_none(), | ||||||
| "ICE: function '{name}' declared subsequentally" | ||||||
| ); | ||||||
|
|
||||||
| let name = self.internal_function_name(name, is_frontend); | ||||||
| let name = if is_frontend { | ||||||
| if let Some(scope) = self.function_scope.last() { | ||||||
| assert!( | ||||||
| !scope.contains_key(name), | ||||||
| "ICE: function '{name}' declared subsequently in the same scope" | ||||||
| ); | ||||||
| } | ||||||
kvpanch marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| let counter = self.function_counter; | ||||||
| self.function_counter += 1; | ||||||
| let mangled = format!("{name}_{}__{counter}", self.code_type().unwrap()); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the double
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Used double underscores to indicate compiler-generated suffix as user less likely use it to name a function.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha 👍 Yeah makes sense |
||||||
| if let Some(scope) = self.function_scope.last_mut() { | ||||||
| scope.insert(name.to_string(), mangled.clone()); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the code above, |
||||||
| } | ||||||
|
Comment on lines
+478
to
+481
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see now why you couldn't remove this If you extract the code type before let name = if is_frontend {
let code_type = self.code_type().unwrap();
let scope = /* ... */;
// ...
let mangled = format!("{name}_{code_type}__{counter}");
scope.insert(name.to_string(), mangled.clone());
// ...
} |
||||||
| mangled | ||||||
| } else { | ||||||
| name.to_string() | ||||||
| }; | ||||||
| let value = self.module().add_function(&name, r#type, linkage); | ||||||
|
|
||||||
| if self.debug_info().is_some() { | ||||||
|
|
@@ -524,9 +542,16 @@ impl<'ctx> Context<'ctx> { | |||||
| name: &str, | ||||||
| is_frontend: bool, | ||||||
| ) -> Option<Rc<RefCell<Function<'ctx>>>> { | ||||||
| self.functions | ||||||
| .get(&self.internal_function_name(name, is_frontend)) | ||||||
| .cloned() | ||||||
| if is_frontend { | ||||||
| let mangled = self | ||||||
| .function_scope | ||||||
| .iter() | ||||||
| .rev() | ||||||
| .find_map(|scope| scope.get(name))?; | ||||||
| self.functions.get(mangled).cloned() | ||||||
| } else { | ||||||
| self.functions.get(name).cloned() | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// Returns a shared reference to the current active function. | ||||||
|
|
@@ -654,6 +679,16 @@ impl<'ctx> Context<'ctx> { | |||||
| .expect("The current context is not in a loop") | ||||||
| } | ||||||
|
|
||||||
| /// Pushes a new function scope. | ||||||
| pub fn push_function_scope(&mut self) { | ||||||
| self.function_scope.push(BTreeMap::new()); | ||||||
| } | ||||||
|
|
||||||
| /// Pops the current function scope. | ||||||
| pub fn pop_function_scope(&mut self) { | ||||||
| self.function_scope.pop(); | ||||||
kvpanch marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| } | ||||||
|
|
||||||
| /// Returns the debug info. | ||||||
| pub fn debug_info(&self) -> Option<&DebugInfo<'ctx>> { | ||||||
| self.debug_info.as_ref() | ||||||
|
|
@@ -1429,15 +1464,6 @@ impl<'ctx> Context<'ctx> { | |||||
| ) | ||||||
| } | ||||||
|
|
||||||
| /// Returns the internal function name. | ||||||
| fn internal_function_name(&self, name: &str, is_frontend: bool) -> String { | ||||||
| if is_frontend { | ||||||
| format!("{name}_{}", self.code_type().unwrap()) | ||||||
| } else { | ||||||
| name.to_string() | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// Scans all functions in the module and removes the `MinSize` attribute | ||||||
| /// if the function contains any large sdiv, udiv, srem, urem instructions with either unknown | ||||||
| /// NOTE: The check here could be relaxed by checking denominator: if the denominator is | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| object "Test" { | ||
| code { | ||
| { | ||
| let size := datasize("Test_deployed") | ||
| codecopy(0, dataoffset("Test_deployed"), size) | ||
| return(0, size) | ||
| } | ||
| } | ||
| object "Test_deployed" { | ||
| code { | ||
| { | ||
| switch calldataload(0) | ||
| case 0 { | ||
| function f() -> ret { | ||
| ret := 1 | ||
| } | ||
|
|
||
| switch calldataload(32) | ||
| case 0 { | ||
| function g() -> ret { | ||
| ret := 10 | ||
| } | ||
| mstore(0, add(f(), g())) | ||
| return(0, 32) | ||
| } | ||
| case 1 { | ||
| function g() -> ret { | ||
| ret := 20 | ||
| } | ||
| mstore(0, add(f(), g())) | ||
| return(0, 32) | ||
| } | ||
| } | ||
| case 1 { | ||
| function f() -> ret { | ||
| ret := 2 | ||
| } | ||
|
|
||
| switch calldataload(32) | ||
| case 0 { | ||
| function g() -> ret { | ||
| ret := 30 | ||
| } | ||
| mstore(0, add(f(), g())) | ||
| return(0, 32) | ||
| } | ||
| case 1 { | ||
| function g() -> ret { | ||
| ret := 40 | ||
| } | ||
| mstore(0, add(f(), g())) | ||
| return(0, 32) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| object "Test" { | ||
| code { | ||
| { | ||
| let size := datasize("Test_deployed") | ||
| codecopy(0, dataoffset("Test_deployed"), size) | ||
| return(0, size) | ||
| } | ||
| } | ||
| object "Test_deployed" { | ||
| code { | ||
| { | ||
| switch calldataload(0) | ||
| case 0 { | ||
| function f() -> ret { | ||
| ret := 1 | ||
| } | ||
| mstore(0, f()) | ||
| return(0, 32) | ||
| } | ||
| case 1 { | ||
| function f() -> ret { | ||
| ret := 2 | ||
| } | ||
| mstore(0, f()) | ||
| return(0, 32) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,3 +9,4 @@ mod remappings; | |
| mod runtime_code; | ||
| mod standard_json; | ||
| mod unsupported_opcodes; | ||
| mod yul_function_scoping; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| //! Tests for Yul function scoping and duplicate function name handling. | ||
|
|
||
| use crate::test_utils::build_yul; | ||
|
|
||
| /// Reproducer from GH-474: duplicate `f` across switch cases. | ||
| #[test] | ||
| fn duplicate_function_names_in_switch_cases() { | ||
| let code = r#" | ||
| object "Test" { | ||
| code { | ||
| { | ||
| let size := datasize("Test_deployed") | ||
| codecopy(0, dataoffset("Test_deployed"), size) | ||
| return(0, size) | ||
| } | ||
| } | ||
| object "Test_deployed" { | ||
| code { | ||
| { | ||
| switch calldataload(0) | ||
| case 0 { | ||
| function f() -> ret { | ||
| ret := 1 | ||
| } | ||
| mstore(0, f()) | ||
| return(0, 32) | ||
| } | ||
| case 1 { | ||
| function f() -> ret { | ||
| ret := 2 | ||
| } | ||
| mstore(0, f()) | ||
| return(0, 32) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| "#; | ||
|
|
||
| build_yul(&[("test.yul", code)]) | ||
| .expect("should compile duplicate function names in switch cases"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: What do you think about prefixing with
frontend_for more clarity?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say it's not necessary as this code belongs to FE. Also, seems naming convention in this file does omit
frontend, except foris_frontend, which I find a bit confusing and maybe better name would beget_mangledor something like that.