Adding a check to verify that functions names do not conflict when translated to assembly labels

[MrDaiki]

Issue

Assembly generation use PrintCommon.escape function to build labels which can creates names conflicts because of namespaces. see (#993).

Solution

We introduce a map in pretyping.ml environment that check if a label is already used by another function.

[vbgl]

The issue only arises for export function. Is it necessary to also fail when one of the functions is internal or inline?

[MrDaiki]

The issue only arises for export function. Is it necessary to also fail when one of the functions is internal or inline?

With this variant of original issue example :

namespace ble {
    fn e() -> reg u64 {
        reg u64 x = 0;
        return x;
    }
}

fn ble__e() -> reg u64 {
    reg u64 x = 1;
    return x;
}

export fn sad() -> reg u64 {
    reg u64 x = ble::e();
    reg u64 y = ble__e();
    return y;
}

generated assembly code is :

        .att_syntax
        .text
        .p2align        5
        .globl  _sad
        .globl  sad
_sad:
sad:
        movq    %rsp, %rsi
        andq    $-8, %rsp
        call    Lble__e$1
Lsad$2:
        call    Lble__e$1
Lsad$1:
        movq    %rsi, %rsp
        ret
Lble__e$1:
        movq    $1, %rax
        ret
Lble__e$1:
        ret

We can see that ble::e assembly code isn't generated, and that the two function calls are done on ble__e. So this issue exist even for internal functions.

I didn't found any bug using inline functions, and I don't expect one since inline dissapears before printasm. I will update the pull request to raise an error only if the function is not inline.

[bgregoir]

I think it will be nicer to not modify the type Env.env.
Write a checker that given a list gfunc ensure that this clash of name do not occurs.
We can call it when we want, maybe at the end of pretyping.

[MrDaiki]

I think it will be nicer to not modify the type Env.env. Write a checker that given a list gfunc ensure that this clash of name do not occurs. We can call it when we want, maybe at the end of pretyping.

I agree with your proposition, but this will require deeper work. For the moment, I will qualify this PR as draft so we can find a suitable (and reusable) solution.

[MrDaiki]

I released a new version applying your suggestion @bgregoir. I think it is ready for review now.

[vbgl]

This should return the list of conflicts instead of failing early.

[vbgl]

This ignore is fishy.

[vbgl]

Also, a warning might be more appropriate.

[MrDaiki]

I did changes as suggested by @vbgl . Since It only print warning now, I didn't saw the interested or returning error list (at least in the current version of error handling). If you think it is better, I can add it.

[vbgl]

• Please add a label_check.mli
• What is the purpose of the test case?

[MrDaiki]

I rewrote error type to be simpler to understand and change the error message to be more clear. I think it is ready to merge now.

[vbgl]

What about type function_label = string L.located

[MrDaiki]

I don't like it but it is also a good type. 'a L.located should only represent located object in my opinion. But I can make the change if you prefer

Benjamin comment has been implemented.