This is an archive of the discontinued LLVM Phabricator instance.

[ExecutionContext] Return the target/process byte order.
ClosedPublic

Authored by JDevlieghere on Jun 27 2018, 9:56 PM.

Details

Summary

Currently ExecutionContext::GetByteOrder() always returns the host byte order. This seems like a simple mistake: the return keyword appears to have been omitted by accident. This patch fixes that and adds a unit test.

Fixes https://llvm.org/PR37950

Diff Detail

Repository
rL LLVM

Event Timeline

ramana-nvr created this revision.Jun 27 2018, 9:56 PM

Created the patch with more context.

clayborg accepted this revision.Jun 28 2018, 9:22 AM
davide requested changes to this revision.Jun 28 2018, 9:23 AM

Can you write a unittest for this? Thanks.

This revision now requires changes to proceed.Jun 28 2018, 9:23 AM

Do we have a test case for different target byte orders that could be used as a test template here? Otherwise I would just merge this in as the previous behavior of this function was clearly wrong.

teemperor set the repository for this revision to rLLDB LLDB.Sep 2 2018, 1:41 PM

Sorry, I don't have a test case for this and I am yet to understand lldb test suite to create one.

My question about a good test case template was more directed at everyone :)

Should be easy to write unit test for ExecutionContext.cpp. See FileSpecTest.cpp for how to implement a unit test for a .cpp file.

JDevlieghere commandeered this revision.Aug 6 2019, 11:21 PM
JDevlieghere updated this revision to Diff 213797.
JDevlieghere added a reviewer: ramana-nvr.

Add unit test

JDevlieghere retitled this revision from [LLDB] Fix for "Bug 37950: ExecutionContext::GetByteOrder() always returns endian::InlHostByteOrder()" to [ExecutionContext] Return the target/process byte order..Aug 6 2019, 11:26 PM
JDevlieghere edited the summary of this revision. (Show Details)
JDevlieghere edited reviewers, added: labath; removed: lldb-commits.
labath accepted this revision.Aug 6 2019, 11:48 PM

Thanks for setting this up. The infrastructure is a bit overkill for a simple test like this, but hopefully this can be useful for other tests too. I have a bunch of comments, but hopefully none of them are major, so if you agree with all of them, just fire away..

lldb/unittests/Target/ExecutionContextTest.cpp
40–43 ↗(On Diff #213797)

call terminate in reverse order?

77 ↗(On Diff #213797)

I don't understand this.. Why do you fetch the ArchSpec via Target::GetDefaultArchitecture() and then immediately overwrite it on the next line? Can this line just be deleted?

78 ↗(On Diff #213797)

It looks weird to have an apple triple with a linux platform. I am assuming you use the linux platform because it can be compiled everywhere, but in that case, maybe you could use a linux triple too? Maybe also use a big-endian triple in one of the tests to catch regressions regardless of the endianness of the host?

85 ↗(On Diff #213797)

change EXPECT_TRUE to ASSERT_TRUE. There's no point in continuing the test if this is false, as it will just crash anyway...

124 ↗(On Diff #213797)

It doesn't look like the presence of a process will change what ExecutionContext::GetByteOrder returns, as it checks the target first. OTOH, I don't know why would the byte order of a process be ever different from the byte order of its target...

JDevlieghere marked 5 inline comments as done.

Code review feedback

JDevlieghere marked 2 inline comments as done.Aug 7 2019, 9:06 AM
JDevlieghere added inline comments.
lldb/unittests/Target/ExecutionContextTest.cpp
77 ↗(On Diff #213797)

Yep, I uploaded an old version of the patch. This and the triple should've already been fixed.

124 ↗(On Diff #213797)

Yeah, I think the logic in the implementation is redundant, this is more of a test to prevent regressions in case the implementation would change in the future.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 7 2019, 9:09 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2019, 9:09 AM