fix: enum-variant names no longer clash #1188

mhasel · 2024-04-03T15:36:43Z

... when codegen runs in parallel.

Enum variant globals are now generated using their qualified name instead of their variant name.

resolves #1182

codecov · 2024-04-03T15:43:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.64%. Comparing base (fedc06d) to head (7400b21).
Report is 4 commits behind head on master.

❗ Current head 7400b21 differs from pull request most recent head d4380df. Consider uploading reports for the commit d4380df to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1188      +/-   ##
==========================================
+ Coverage   95.62%   95.64%   +0.02%     
==========================================
  Files         150      148       -2     
  Lines       42657    42533     -124     
==========================================
- Hits        40789    40682     -107     
+ Misses       1868     1851      -17

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

... when codegen runs parallel. Enum variant globals are now generated using their qualified name instead of their variant name. Resolves #1182

mhasel · 2024-04-04T09:17:01Z

src/codegen/generators/variable_generator.rs

+                global_variable.get_name()
+            };
+
+        let mut global_ir_variable = self.llvm.create_global_variable(self.module, name, variable_type);
        if linkage == LinkageType::External {
            global_ir_variable = global_ir_variable.make_external();


can enums be linked externally? if so, I'll need to exclude externally linked enums in the above conditional

you can have enums as part of a library so I would say yes

Just out of curiousity, how would the declaration look like?

Compiling the following on master (taking a guess on this declaration):

{external} VAR_GLOBAL myEnum: (a,b); END_VAR

results in this IR:

@myEnum= external global i32 @a = unnamed_addr constant i32 0 @b = unnamed_addr constant i32 1

The variants are defined as local constants - wouldn't they clash with the external ones, if it had different values?
What would happen if the linked enum looked like this:

myEnum: (a := 29, b);

edit:
I have tried this example with a slightly modified version of the example in the next comment:

{external} VAR_GLOBAL Variable: (a,b); (* Comment *) END_VAR FUNCTION main : DINT VAR x: DINT; END_VAR x := Variable#a; END_FUNCTION

The library part stayed the same.

on master it fails with
error[E071]: Linking globals named 'a': symbol multiply defined!

on this branch it compiles and links, but now x will have the value 0.
So I guess that is a problem and we want linking to fail here?

That presents another problem: if the library is also built with rusty, the enum will have the qualified name (it isnt external), but the externally declared enum will not. So the result ends up being the same: no linking error, x will have the value 0.

Should external inline enums work? Or just declared types?

I went ahead and tried it:
enum "library":

extEnum.st

TYPE extEnum : (a := 29, b); END_TYPE

compiled with cargo r -- target/extEnum.st --shared -o libextEnum.so

demo.st

{external} VAR_GLOBAL Variable: extEnum; (* Comment *) END_VAR FUNCTION main : DINT VAR x: DINT; END_VAR x := extEnum.a; END_FUNCTION

compiled and linked with LD_LIBRARY_PATH=. cargo r -- target/demo.st -i target/extEnum.st -lextEnum -L. --ir

resulting IR:
master

; ModuleID = '/tmp/.tmpCBozkp/target/demo.st.ll' source_filename = "target/demo.st" @Variable = external global i32 @a = constant i32 29 @b = unnamed_addr constant i32 30 define i32 @main() { entry: %main = alloca i32, align 4 %x = alloca i32, align 4 store i32 0, i32* %x, align 4 store i32 0, i32* %main, align 4 store i32 29, i32* %x, align 4 %main_ret = load i32, i32* %main, align 4 ret i32 %main_ret }

this branch

; ModuleID = '/tmp/.tmpVy0Kpa/target/demo.st.ll' source_filename = "target/demo.st" @Variable = external global i32 @extEnum.a = constant i32 29 @extEnum.b = unnamed_addr constant i32 30 define i32 @main() { entry: %main = alloca i32, align 4 %x = alloca i32, align 4 store i32 0, i32* %x, align 4 store i32 0, i32* %main, align 4 store i32 29, i32* %x, align 4 %main_ret = load i32, i32* %main, align 4 ret i32 %main_ret }

It looks like the constants are resolved/folded correctly with both approaches, so not differentiating should be fine?

edit:
I've noticed that I've used extEnum.a instead of Variable.a so I recompiled both examples using the declared global:

{external} VAR_GLOBAL Variable: extEnum; (* Comment *) END_VAR FUNCTION main : DINT VAR x: DINT; END_VAR x := Variable.a; END_FUNCTION

The IR stayed the same and the constants are folded correctly. I have also tried to compile the library on master and the demo file on this branch, doesnt seem to make a difference. Am I doing something wrong here?

Ok, lt looks like since this is a constant the folding is happening correctly and therefore no conflicts arise. My assumption was this will behave similarly to a global variable being defined twice which is where the external global ... comes into play

My assumption was similar, hence the original question. Now, after trying it out with both an externally defined inline-enum and enum-type (see my findings above - I have edited/updated both posts), a different question arose:
Should we just add a validation against externally declared inline enums? types dont seem problematic, but if we want inline-enums to "work" (it will fail during linking due to multiply defined symbols right now regardless) with the "external" pragma we need a different approach to mangling.

Maybe I misunderstood something but wouldn't this commit now ensure the enums will not conflict? Even if they are external? Why would the linking fail if the enums have their own unique names?

Let me try to rephrase:
Externally defined enum types work as expected, my concern were inline enums:
Inline declared enums would take the value of the locally defined variant, even when decorated with the external pragma (making the pragma essentially pointless).
I thought it would be better to validate against this instead of potentially have misleading behaviour.
However, I noticed that if the externally declared enum is also an inline enum, the changes from #1175 now detect it as ambigious datatype during validation, so this point is moot.

ghaith · 2024-04-04T14:30:48Z

src/codegen/generators/variable_generator.rs

+        let name = if self.global_index.get_type_information_or_void(type_name).is_enum() {
+            global_variable.get_qualified_name()
+        } else {
+            global_variable.get_name()


Anything against just using the get_quailified_name here? I think for now it should deliver the same for global variables

Good point, I'll give it a try

This approach will break tests/code depending on inkwell::add_global_function_mapping. Subsequent calls to ExecutionEngine::get_function will then be unable to find the given functions by name in GeneratedModule::run and cause tests to panic on unwrap.

Somehow this seems to affect PROGRAMs but not FUNCTION_BLOCKs or FUNCTIONs, I guess since PROGRAMs are global by default?

PROGRAM foo END_PROGRAM FUNCTION_BLOCK bar END_FUNCTION_BLOCK FUNCTION baz END_FUNCTION

%foo = type {} %bar = type {} @foo = global %foo zeroinitializer @__bar__init = unnamed_addr constant %bar zeroinitializer define void @foo.1(%foo* %0) { entry: ret void } define void @bar(%bar* %0) { entry: ret void } define void @baz() { entry: ret void }

foo is now being declared as foo.1 - that's quite an interesting side-effect.
I can try filtering programs instead of enums and see if that fixes it/if there are then other side-effects

yeah foo.1 would not be good as it probably won't be a reliable result for the test, also that meens calling the function from C will have issue i assume. Shouldn't the global initializer for foo be called something else? we usually called it foo_instance or __foo__init or something similar. This is probably what qualified name is not doing.. interesting

Filtering out programs instead of enums seems promising, but I need to write additional tests. These would involve linking against a precompiled library which exposes the globals and then check for expected parameters. I got sidetracked while working on another issue, but I'll try to get on this tomorrow, so we can merge/close this.

Let's pair on this before you change the behavior / filter out programs.

fix: enum-variant names no longer clash

0c6a029

... when codegen runs parallel. Enum variant globals are now generated using their qualified name instead of their variant name. Resolves #1182

mhasel force-pushed the enum-name-conflicts branch from c296505 to 0c6a029 Compare April 4, 2024 09:14

mhasel commented Apr 4, 2024

View reviewed changes

mhasel marked this pull request as ready for review April 4, 2024 09:17

mhasel requested a review from ghaith April 4, 2024 09:23

mhasel and others added 3 commits April 4, 2024 11:30

Merge branch 'master' into enum-name-conflicts

d2e57b2

insta

7368789

fmt

7400b21

ghaith reviewed Apr 4, 2024

View reviewed changes

mhasel and others added 4 commits April 8, 2024 08:57

Merge branch 'master' into enum-name-conflicts

6bf88ed

Merge branch 'master' into enum-name-conflicts

c5fba10

Merge branch 'master' into enum-name-conflicts

2e63031

Merge branch 'master' into enum-name-conflicts

d4380df

ghaith approved these changes Apr 18, 2024

View reviewed changes

mhasel merged commit 66ce86a into master Apr 18, 2024
15 checks passed

mhasel deleted the enum-name-conflicts branch April 18, 2024 07:36

mhasel mentioned this pull request Apr 18, 2024

Write additional external linkage/runtime tests with LIT once it is introduced #1204

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: enum-variant names no longer clash #1188

fix: enum-variant names no longer clash #1188

mhasel commented Apr 3, 2024 •

edited

Loading

codecov bot commented Apr 3, 2024 •

edited

Loading

mhasel Apr 4, 2024

ghaith Apr 4, 2024

mhasel Apr 4, 2024 •

edited

Loading

mhasel Apr 4, 2024 •

edited

Loading

ghaith Apr 4, 2024

mhasel Apr 4, 2024 •

edited

Loading

ghaith Apr 4, 2024

mhasel Apr 5, 2024 •

edited

Loading

ghaith Apr 4, 2024

mhasel Apr 5, 2024

mhasel Apr 5, 2024

ghaith Apr 15, 2024

mhasel Apr 15, 2024

ghaith Apr 17, 2024

fix: enum-variant names no longer clash #1188

fix: enum-variant names no longer clash #1188

Conversation

mhasel commented Apr 3, 2024 • edited Loading

codecov bot commented Apr 3, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhasel Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

mhasel Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhasel Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhasel Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhasel commented Apr 3, 2024 •

edited

Loading

codecov bot commented Apr 3, 2024 •

edited

Loading

mhasel Apr 4, 2024 •

edited

Loading

mhasel Apr 4, 2024 •

edited

Loading

mhasel Apr 4, 2024 •

edited

Loading

mhasel Apr 5, 2024 •

edited

Loading