Provide unique name from closures with same name#488
Conversation
In YUL it's possible to declare closure with a same name, but in different scope, which previously resulted to func name conflict during LLVM IR construction. To solve this, track YUL scope when building LLVM IR and a special counter that increments every time function with the same name exists in LLVM module. There's still assert if somehow within the same scope there 2 functions with the same name, which should be forbidden by Yul Fixes: #474
| /// 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>>, |
There was a problem hiding this comment.
Nit: What do you think about prefixing with frontend_ for more clarity?
There was a problem hiding this comment.
I'd say it's not necessary as this code belongs to FE. Also, seems naming convention in this file does omit frontend, except for is_frontend, which I find a bit confusing and maybe better name would be get_mangled or something like that.
| } | ||
| let counter = self.function_counter; | ||
| self.function_counter += 1; | ||
| let mangled = format!("{name}_{}__{counter}", self.code_type().unwrap()); |
There was a problem hiding this comment.
Is the double _ intentional here?
| let mangled = format!("{name}_{}__{counter}", self.code_type().unwrap()); | |
| let mangled = format!("{name}_{}_{counter}", self.code_type().unwrap()); |
There was a problem hiding this comment.
Used double underscores to indicate compiler-generated suffix as user less likely use it to name a function.
Since we have rules to mangle function names, we can make one: even have $ here instead of __ which would help later to debug code
Co-authored-by: LJ <81748770+elle-j@users.noreply.github.com>
Co-authored-by: LJ <81748770+elle-j@users.noreply.github.com>
| let mangled = format!("{name}_{}__{counter}", self.code_type().unwrap()); | ||
| if let Some(scope) = self.function_scope.last_mut() { | ||
| scope.insert(name.to_string(), mangled.clone()); | ||
| } |
There was a problem hiding this comment.
Ah, I see now why you couldn't remove this if let Some() as part of this commit in order to just scope.insert(..) directly.
If you extract the code type before let scope, you'll be able to replace it 👍
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());
// ...
}
xermicus
left a comment
xermicus
left a comment
There was a problem hiding this comment.
Thanks! The logic looks good to me, couldn't find an obvious issue. However some integration tests with mangled functions, to ensure the mangling logic works correctly, would be nice to have
| let scope = self | ||
| .function_scope | ||
| .last_mut() | ||
| .expect("ICE: function scope must be pushed before declaring frontend functions"); |
There was a problem hiding this comment.
I prefer this to be a getter function like current_function_scope (ensures same access / error pattern for future use)
| self.function_counter += 1; | ||
| let mangled = format!("{name}_{}__{counter}", self.code_type().unwrap()); | ||
| if let Some(scope) = self.function_scope.last_mut() { | ||
| scope.insert(name.to_string(), mangled.clone()); |
There was a problem hiding this comment.
Looking at the code above, last_mut() returning None here is unreachable making the if let redundant.
3 participants
In YUL it's possible to declare closure with a same name, but in different scope, which previously resulted to func name conflict during LLVM IR construction.
To solve this, track YUL scope when building LLVM IR and a special counter that increments every time function with the same name exists in LLVM module.
There's still assert if somehow within the same scope there 2 functions with the same name, which should be forbidden by Yul
Fixes: #474