Skip to content

Functions generated by Function::createWithDefaultAttr should respect -target-features #93633

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

Closed
pcc opened this issue May 29, 2024 · 1 comment · Fixed by #96721
Closed

Functions generated by Function::createWithDefaultAttr should respect -target-features #93633

pcc opened this issue May 29, 2024 · 1 comment · Fixed by #96721
Labels
clang:codegen IR generation bugs: mangling, exceptions, etc. llvm:ir

Comments

@pcc
Copy link
Contributor

pcc commented May 29, 2024

The following example usage of coverage + HWASan + LTO will fail:

$ cat test.cc
__attribute__((weak)) bool foo = false;

__attribute__((weak)) void bar() {}

int main() {
  if (foo) bar();
}
$ clang test.cc -O3 --target=aarch64-linux-android30 -flto -fsanitize=hwaddress -coverage --sysroot=$NDK/toolchains/llvm/prebuilt/linux-x86_64/sysroot --gcc-toolchain=$NDK/toolchains/llvm/prebuilt/linux-x86_64
ld.lld: error: a.out.lto.o:(function __llvm_gcov_writeout: .text.__llvm_gcov_writeout+0x10): relocation R_AARCH64_ADR_PREL_PG_HI21 out of range: 9079256848778895360 is not in [-4294967296, 4294967295]; references section '.rodata..L.hwasan'
>>> referenced by ld-temp.o

ld.lld: error: a.out.lto.o:(function __llvm_gcov_writeout: .text.__llvm_gcov_writeout+0x3c): relocation R_AARCH64_ADR_PREL_PG_HI21 out of range: 8935141660703096832 is not in [-4294967296, 4294967295]; references section '.data..L__llvm_gcov_ctr.hwasan'
>>> referenced by ld-temp.o

ld.lld: error: a.out.lto.o:(function __llvm_gcov_writeout: .text.__llvm_gcov_writeout+0x64): relocation R_AARCH64_ADR_PREL_PG_HI21 out of range: 9007199254741024768 is not in [-4294967296, 4294967295]; references section '.bss..L__llvm_gcov_ctr.1.hwasan'
>>> referenced by ld-temp.o

ld.lld: error: a.out.lto.o:(function __llvm_gcov_reset: .text.__llvm_gcov_reset+0x0): relocation R_AARCH64_ADR_PREL_PG_HI21 out of range: 8935141660703096832 is not in [-4294967296, 4294967295]; references section '.data..L__llvm_gcov_ctr.hwasan'
>>> referenced by ld-temp.o

ld.lld: error: a.out.lto.o:(function __llvm_gcov_reset: .text.__llvm_gcov_reset+0xc): relocation R_AARCH64_ADR_PREL_PG_HI21 out of range: 9007199254741024768 is not in [-4294967296, 4294967295]; references section '.bss..L__llvm_gcov_ctr.1.hwasan'
>>> referenced by ld-temp.o

The cause is that the compiler-generated functions __llvm_gcov_writeout and __llvm_gcov_reset do not have target-features=+tagged-globals attributes. These functions are created by Function::createWithDefaultAttr. The general form of this problem is known, and as such there's already an attempt in that function to copy certain attributes from the module flags onto the function. But for target features there's no module flag since the whole point of target features is that they're per-function.

I feel like this is probably a bug in Function::createWithDefaultAttr (which GCOV uses to create its compiler generated functions). It should probably set the target-features attribute to a list that it reads from the LLVMContext, or something like that (which Clang would fill in before running the pass pipeline). As things are, these generated functions won't respect things like -ffixed-x* flags either. That would imply that Function::createWithDefaultAttr shouldn't be used during LTO or in the backend. If looks like that's mostly already the case except for a couple of the GPU backends that are calling this function to create constructors.

pcc added a commit that referenced this issue Jun 26, 2024
…d via createWithDefaultAttr().

Functions created with createWithDefaultAttr() need to have the
correct target-{cpu,features} attributes to avoid miscompilations
such as using the wrong relocation type to access globals (missing
tagged-globals feature), clobbering registers specified via -ffixed-*
(missing reserve-* feature), and so on.

There's already a number of attributes copied from the module flags
onto functions created by createWithDefaultAttr(). I don't think
module flags are the right choice for the target attributes because
we don't need the conflict resolution logic between modules with
different target attributes, nor does it seem sensible to add it:
there's no unambiguously "correct" set of target attributes when
merging two modules with different attributes, and nor should there
be; it's perfectly valid for two modules to be compiled with different
target attributes, that's the whole reason why they are per-function.

This also implies that it's unnecessary to serialize the attributes in
bitcode, which implies that they shouldn't be stored on the module. We
can also observe that for the most part, createWithDefaultAttr()
is called from compiler passes such as sanitizers, coverage and
profiling passes that are part of the compile time pipeline, not
the LTO pipeline. This hints at a solution: we need to store the
attributes in a non-serialized location associated with the ambient
compilation context. Therefore in this patch I elected to store the
attributes on the LLVMContext.

There are calls to createWithDefaultAttr() in the NVPTX and AMDGPU
backends, and those calls would happen at LTO time. For those callers,
the bug still potentially exists and it would be necessary to refactor
them to create the functions at compile time if this issue is relevant
on those platforms.

Fixes #93633.

Reviewers: fmayer, MaskRay, eugenis

Reviewed By: MaskRay

Pull Request: #96721
@EugeneZelenko EugeneZelenko added clang:codegen IR generation bugs: mangling, exceptions, etc. llvm:ir and removed new issue labels Jun 26, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 26, 2024

@llvm/issue-subscribers-clang-codegen

Author: None (pcc)

The following example usage of coverage + HWASan + LTO will fail: ``` $ cat test.cc __attribute__((weak)) bool foo = false;

attribute((weak)) void bar() {}

int main() {
if (foo) bar();
}
$ clang test.cc -O3 --target=aarch64-linux-android30 -flto -fsanitize=hwaddress -coverage --sysroot=$NDK/toolchains/llvm/prebuilt/linux-x86_64/sysroot --gcc-toolchain=$NDK/toolchains/llvm/prebuilt/linux-x86_64
ld.lld: error: a.out.lto.o:(function __llvm_gcov_writeout: .text.__llvm_gcov_writeout+0x10): relocation R_AARCH64_ADR_PREL_PG_HI21 out of range: 9079256848778895360 is not in [-4294967296, 4294967295]; references section '.rodata..L.hwasan'
>>> referenced by ld-temp.o

ld.lld: error: a.out.lto.o:(function __llvm_gcov_writeout: .text.__llvm_gcov_writeout+0x3c): relocation R_AARCH64_ADR_PREL_PG_HI21 out of range: 8935141660703096832 is not in [-4294967296, 4294967295]; references section '.data..L__llvm_gcov_ctr.hwasan'
>>> referenced by ld-temp.o

ld.lld: error: a.out.lto.o:(function __llvm_gcov_writeout: .text.__llvm_gcov_writeout+0x64): relocation R_AARCH64_ADR_PREL_PG_HI21 out of range: 9007199254741024768 is not in [-4294967296, 4294967295]; references section '.bss..L__llvm_gcov_ctr.1.hwasan'
>>> referenced by ld-temp.o

ld.lld: error: a.out.lto.o:(function __llvm_gcov_reset: .text.__llvm_gcov_reset+0x0): relocation R_AARCH64_ADR_PREL_PG_HI21 out of range: 8935141660703096832 is not in [-4294967296, 4294967295]; references section '.data..L__llvm_gcov_ctr.hwasan'
>>> referenced by ld-temp.o

ld.lld: error: a.out.lto.o:(function __llvm_gcov_reset: .text.__llvm_gcov_reset+0xc): relocation R_AARCH64_ADR_PREL_PG_HI21 out of range: 9007199254741024768 is not in [-4294967296, 4294967295]; references section '.bss..L__llvm_gcov_ctr.1.hwasan'
>>> referenced by ld-temp.o

The cause is that the compiler-generated functions `__llvm_gcov_writeout` and `__llvm_gcov_reset` do not have `target-features=+tagged-globals` attributes. These functions are created by `Function::createWithDefaultAttr`. The general form of this problem is known, and as such there's already an attempt in that function to copy certain attributes from the module flags onto the function. But for target features there's no module flag since the whole point of target features is that they're per-function.

I feel like this is probably a bug in `Function::createWithDefaultAttr` (which GCOV uses to create its compiler generated functions). It should probably set the target-features attribute to a list that it reads from the LLVMContext, or something like that (which Clang would fill in before running the pass pipeline). As things are, these generated functions won't respect things like `-ffixed-x*` flags either. That would imply that `Function::createWithDefaultAttr` shouldn't be used during LTO or in the backend. If looks like that's mostly already the case except for a couple of the GPU backends that are calling this function to create constructors.
</details>

AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this issue Jul 9, 2024
…d via createWithDefaultAttr().

Functions created with createWithDefaultAttr() need to have the
correct target-{cpu,features} attributes to avoid miscompilations
such as using the wrong relocation type to access globals (missing
tagged-globals feature), clobbering registers specified via -ffixed-*
(missing reserve-* feature), and so on.

There's already a number of attributes copied from the module flags
onto functions created by createWithDefaultAttr(). I don't think
module flags are the right choice for the target attributes because
we don't need the conflict resolution logic between modules with
different target attributes, nor does it seem sensible to add it:
there's no unambiguously "correct" set of target attributes when
merging two modules with different attributes, and nor should there
be; it's perfectly valid for two modules to be compiled with different
target attributes, that's the whole reason why they are per-function.

This also implies that it's unnecessary to serialize the attributes in
bitcode, which implies that they shouldn't be stored on the module. We
can also observe that for the most part, createWithDefaultAttr()
is called from compiler passes such as sanitizers, coverage and
profiling passes that are part of the compile time pipeline, not
the LTO pipeline. This hints at a solution: we need to store the
attributes in a non-serialized location associated with the ambient
compilation context. Therefore in this patch I elected to store the
attributes on the LLVMContext.

There are calls to createWithDefaultAttr() in the NVPTX and AMDGPU
backends, and those calls would happen at LTO time. For those callers,
the bug still potentially exists and it would be necessary to refactor
them to create the functions at compile time if this issue is relevant
on those platforms.

Fixes llvm#93633.

Reviewers: fmayer, MaskRay, eugenis

Reviewed By: MaskRay

Pull Request: llvm#96721
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen IR generation bugs: mangling, exceptions, etc. llvm:ir
Projects
None yet
3 participants