[LLD] Invalid 'there is a cycle in linker script INCLUDEs' error

[partaror]

LLD incorrectly reports 'there is a cycle in linker script INCLUDEs' error when a linker script is included multiple times (but there is no cycle). Reproducible example:

#!/usr/bin/env bash

cat > incrementDotBy100.t \EOF
  . = . + 100;
EOF

cat >1.t <<\EOF
SECTIONS {
  FOO : {
    *(*foo*)
    INCLUDE incrementDotBy100.t
  }
  BAR : {
    *(*bar*)
    INCLUDE incrementDotBy100.t
  }
}
EOF

touch 1.c
clang-15 -o 1.o 1.c -c
ld.lld -o 1.elf 1.o -T 1.t

Output:

ld.lld: error: 1.t:8: there is a cycle in linker script INCLUDEs
>>>     INCLUDE incrementDotBy100.t
>>> 

[llvmbot]

@llvm/issue-subscribers-lld-elf

Author: Parth Arora (partaror)

LLD incorrectly reports 'there is a cycle in linker script INCLUDEs' error when a linker script is included multiple times (but there is no cycle). Reproducible example:

#!/usr/bin/env bash

cat > incrementDotBy100.t \EOF
  . = . + 100;
EOF

cat >1.t <<\EOF
SECTIONS {
  FOO : {
    *(*foo*)
    INCLUDE incrementDotBy100.t
  }
  BAR : {
    *(*bar*)
    INCLUDE incrementDotBy100.t
  }
}
EOF

touch 1.c
clang-15 -o 1.o 1.c -c
ld.lld -o 1.elf 1.o -T 1.t

Output:

ld.lld: error: 1.t:8: there is a cycle in linker script INCLUDEs
>>>     INCLUDE incrementDotBy100.t
>>> 

[MaskRay]

The side effect of https://reviews.llvm.org/D37524 is that a file cannot be included twice.
When is a file included twice a reasonable use case? That rules out output sections and symbol assignments (duplicate definitions).
It seems that only dot advancement makes sense, as your test case demonstrates.

Fixing this require an include stack and an action upon included file EOF.

GNU ld does not detect cycles. It has a %F:includes nested too deeply\n error instead.

[partaror]

Thank you for the reply @MaskRay.

Fixing this require an include stack and an action upon included file EOF.

Yes, I agree that it may not be worth to fix this issue. At least not unless there is any practical benefit.

However, I believe we should correct the error message for the sake of correctness. What do you think about changing the error message to: Multiple includes of the same file are not allowed

[mysterymath]

Isn't this the kind of thing that we could do in readInclude? It seems like it would just be a matter of removing the token from seen before returning from the function.

[partaror]

@mysterymath No, we cannot do this in readInclude because readInclude only adds the token from the include file to the main buffer, it does not process the tokens. So, if an include file has an INCLUDE command then readInclude function would not know about it -- therefore, we cannot catch recursive includes in readInclude function.

[MaskRay]

multiple ... (no capitalization) looks good.

[quicvede]

@MaskRay @partaror pushed a change to update the error message #94861

[MaskRay]

I've created #100493 to add an include stack. I'll fix this properly once it's merged.