-
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 3 commits
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,26 @@ 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 { | ||||||
| let scope = self | ||||||
| .function_scope | ||||||
| .last_mut() | ||||||
| .expect("ICE: function scope must be pushed before declaring frontend functions"); | ||||||
kvpanch marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
|
|
||||||
| assert!( | ||||||
| !scope.contains_key(name), | ||||||
| "ICE: function '{name}' declared subsequently in the same scope" | ||||||
| ); | ||||||
| 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()); | ||||||
kvpanch marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| } | ||||||
|
||||||
| mangled | ||||||
| } else { | ||||||
| name.to_string() | ||||||
| }; | ||||||
| let value = self.module().add_function(&name, r#type, linkage); | ||||||
|
|
||||||
| if self.debug_info().is_some() { | ||||||
|
|
@@ -524,9 +545,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 +682,18 @@ 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() | ||||||
| .expect("ICE: tried to pop an empty function scope stack"); | ||||||
| } | ||||||
|
|
||||||
| /// Returns the debug info. | ||||||
| pub fn debug_info(&self) -> Option<&DebugInfo<'ctx>> { | ||||||
| self.debug_info.as_ref() | ||||||
|
|
@@ -1429,15 +1469,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.