Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Weird behavior of ELF tests with newer version of MSVC. #60013

Closed
rlavaee opened this issue Jan 13, 2023 · 2 comments · Fixed by #79168
Closed

Weird behavior of ELF tests with newer version of MSVC. #60013

rlavaee opened this issue Jan 13, 2023 · 2 comments · Fixed by #79168
Assignees
@red1bluelost
Labels
llvm Umbrella label for LLVM issues platform:windows test-suite

Comments

@rlavaee
Copy link
Contributor

rlavaee commented Jan 13, 2023

As pointed in https://reviews.llvm.org/D100808, ELF format tests succeed with MSVC v14.29 (from Visual Studio 2019) but fail with MSVC v14.33 (from Visual Studio 2022).
@EugeneZelenko EugeneZelenko added test-suite platform:windows llvm Umbrella label for LLVM issues and removed new issue labels Jan 13, 2023
@red1bluelost red1bluelost mentioned this issue Jan 23, 2024
Merged
@red1bluelost
Copy link
Member

Testing on llvm-objdump/X86/elf-bbaddrmap-disassemble-symbolize-operands.yaml, I've managed to bisect the issue as being a yaml2obj problem and not llvm-objdump/readBBAddrMap. I'm assigning this to me as I should be able to get around to debugging more this week.

@red1bluelost red1bluelost self-assigned this Jan 23, 2024
@red1bluelost red1bluelost mentioned this issue Jan 23, 2024
Merged
@red1bluelost
Copy link
Member

I have a fix up at #79168. The problem is we accidentally had side-effects in the expressions of a binary operator. This is unfortunately unspecified behavior in C++ which is why the generated ELF would have fields out of order.

@red1bluelost red1bluelost linked a pull request Jan 23, 2024 that will close this issue
[SHT_LLVM_BB_ADDR_MAP] Avoids side-effects in addition since order is unspecified. #79168
Merged
red1bluelost added a commit that referenced this issue Jan 24, 2024
@red1bluelost
[SHT_LLVM_BB_ADDR_MAP] Avoids side-effects in addition since order is…
23faa81 
… unspecified. (#79168)

Turns out the problem with
#60013 is due to the fact
that order of operation is unspecified in C++:
https://en.cppreference.com/w/cpp/language/eval_order. A small example
of where this manifests with MSVC can be seen here
https://ooo.godbolt.org/z/bxqKeqzqn.

This patch does the following:
* Removes the addition operations where we sequence more than one
side-effect based expression.
* Removes test guards to now run on Windows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@red1bluelost red1bluelost

Labels
llvm Umbrella label for LLVM issues platform:windows test-suite
Projects
None yet
Milestone
No milestone
3 participants
@rlavaee @EugeneZelenko @red1bluelost