Skip to content

llvm-readobj reads incorrect values from AMD note property on big-endian hosts #65280

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
kovdan01 opened this issue Sep 4, 2023 · 5 comments · Fixed by #70775
Closed

llvm-readobj reads incorrect values from AMD note property on big-endian hosts #65280

kovdan01 opened this issue Sep 4, 2023 · 5 comments · Fixed by #70775

Comments

@kovdan01
Copy link
Contributor

kovdan01 commented Sep 4, 2023

getAMDNote function from llvm/tools/llvm-readobj/ELFDumper.cpp fails to read values from the AMD note properly on big-endian hosts. This is result of reinterpret_cast which is followed by reading integers without proper handling of different endianness.

Possible fix might include use of a special wrapper for integers which handles different endianness implicitly, like support::detail::packed_endian_specific_integral.

FAIL: LLVM :: tools/llvm-readobj/ELF/note-amd-valid-v2.test (1 of 293)
******************** TEST 'LLVM :: tools/llvm-readobj/ELF/note-amd-valid-v2.test' FAILED ********************
Exit Code: 1
                                                                                                                                                                                                                                                            
Command Output (stderr):
--
+ : 'RUN: at line 7'
+ /path/to/llvm-project/.build/aarch64_be-rel-with-deb-info/bin/yaml2obj /path/to/llvm-project/llvm/test/tools/llvm-readobj/ELF/note-amd-valid-v2.test -o /path/to/llvm-project/.build/aarch64_be-rel-
with-deb-info/test/tools/llvm-readobj/ELF/Output/note-amd-valid-v2.test.tmp.o
+ : 'RUN: at line 8'
+ /path/to/llvm-project/.build/aarch64_be-rel-with-deb-info/bin/llvm-readobj --notes /path/to/llvm-project/.build/aarch64_be-rel-with-deb-info/test/tools/llvm-readobj/ELF/Output/note-amd-valid-v2.test.tmp.o
+ /path/to/llvm-project/.build/aarch64_be-rel-with-deb-info/bin/FileCheck /path/to/llvm-project/llvm/test/tools/llvm-readobj/ELF/note-amd-valid-v2.test --match-full-lines --check-prefix=LLVM
/path/to/llvm-project/llvm/test/tools/llvm-readobj/ELF/note-amd-valid-v2.test:20:14: error: LLVM-NEXT: expected string not found in input
# LLVM-NEXT: AMD HSA Code Object Version: [Major: 2, Minor: 1]
             ^
<stdin>:15:68: note: scanning from here
 Type: NT_AMD_HSA_CODE_OBJECT_VERSION (AMD HSA Code Object Version)
                                                                   ^
<stdin>:16:2: note: possible intended match here
 AMD HSA Code Object Version: [Major: 33554432, Minor: 16777216]
 ^
                                                                                                                                                                                                                                                            
Input file: <stdin>
Check file: /path/to/llvm-project/llvm/test/tools/llvm-readobj/ELF/note-amd-valid-v2.test                                                                                                                         
@kovdan01 kovdan01 changed the title [AArch64] llvm-readobj reads incorrect values from AMD note property on big-endian hosts llvm-readobj reads incorrect values from AMD note property on big-endian hosts Sep 4, 2023
@llvmbot
Copy link
Member

llvmbot commented Sep 4, 2023

@llvm/issue-subscribers-backend-amdgpu

Pierre-vh added a commit to Pierre-vh/llvm-project that referenced this issue Oct 31, 2023
@Pierre-vh
Copy link
Contributor

@kovdan01 Can you please check if #70775 fixes it?
I don't have a big endian system to confirm the fix.

@kovdan01
Copy link
Contributor Author

Thanks @Pierre-vh , I'll let you know when I check it: don't have an immediate access to a big-endian system right now, so I'll check the fix a bit later.

@arsenm
Copy link
Contributor

arsenm commented Nov 14, 2023

I think it's OK to just merge the PR now and assume it's fixed

Pierre-vh added a commit that referenced this issue Nov 14, 2023
Should avoid issues on big-endian hosts.

Note that we use aligned types because primitive integers are also
aligned. If we don't use aligned types, `HSAILProperties` ends up being
11 bytes instead of 12 (1 byte padding at the end of the struct added by
the compiler).
Technically only the first type needs to be aligned, but I just used
aligned types everywhere to be consistent.

Fixes #65280
@Pierre-vh
Copy link
Contributor

It's indeed fixed, we had a XFAIL on big endian for the note reading test and it passed, so a buildbot complained. I didn't even know we had a BE buildbot, nice! :)
https://lab.llvm.org/buildbot/#/builders/231/builds/17994

zahiraam pushed a commit to zahiraam/llvm-project that referenced this issue Nov 20, 2023
…70775)

Should avoid issues on big-endian hosts.

Note that we use aligned types because primitive integers are also
aligned. If we don't use aligned types, `HSAILProperties` ends up being
11 bytes instead of 12 (1 byte padding at the end of the struct added by
the compiler).
Technically only the first type needs to be aligned, but I just used
aligned types everywhere to be consistent.

Fixes llvm#65280
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants