This is an archive of the discontinued LLVM Phabricator instance.

Differential D83661

[llvm][unittests] Fix LLVM-Unit :: Support/./SupportTests/ProgramEnvTest.TestExecuteAndWaitStatistics on Solaris
ClosedPublic

Authored by ro on Jul 13 2020, 2:34 AM.

Details

Reviewers
sepavloff
zturner
aprantl
aganea
Group Reviewers
Restricted Project
Commits
rG2d1fe0c37247: [llvm][unittests] Fix ProgramEnvTest.TestExecuteAndWaitStatistics on Solaris
Summary

The new LLVM-Unit :: Support/./SupportTests/ProgramEnvTest.TestExecuteAndWaitStatistics test currently FAILs on Solaris:

[ RUN      ] ProgramEnvTest.TestExecuteAndWaitStatistics
/vol/llvm/src/llvm-project/local/llvm/unittests/Support/ProgramTest.cpp:360: Failure
Expected: (ProcStat->PeakMemory) > (0U), actual: 0 vs 0
[  FAILED  ] ProgramEnvTest.TestExecuteAndWaitStatistics (22 ms)

According to llvm/lib/Support/Unix/Program.inc (llvm::sys::Wait), PeakMemory
corresponds to struct rusage.ru_maxrss.

However, Solaris getrusage(3C) documents

NOTES
       The ru_maxrss, ru_ixrss, ru_idrss, and ru_isrss members of  the  rusage
       structure are set to 0 in this implementation.

To deal with this, this patch changes to test to allow for PeakMemory == 0.

Tested on amd64-pc-solaris2.11. Ok for master?

Diff Detail

Event Timeline

ro created this revision.Jul 13 2020, 2:34 AM
Herald added a subscriber: fedor.sergeev. · View Herald TranscriptJul 13 2020, 2:34 AM
Harbormaster failed remote builds in B63924: Diff 277351!Jul 13 2020, 3:05 AM
sepavloff added inline comments.Jul 13 2020, 9:07 AM
llvm/unittests/Support/ProgramTest.cpp
359–360

PeakMemory is uint64_t, so the check condition is always true. It may cause compiler warnings.

I would propose to remove this check at all.

ro updated this revision to Diff 277557.Jul 13 2020, 1:43 PM

Remove ProcStat->PeakMemory check: pointless now.

ro marked 2 inline comments as done.Jul 13 2020, 1:45 PM
ro added inline comments.
llvm/unittests/Support/ProgramTest.cpp
359–360

On Solaris (and the BSDs where getrusage originated), it's
long instead, thus no warning.

But I agree: the check is pretty pointless now.

Harbormaster failed remote builds in B64041: Diff 277557!Jul 13 2020, 2:14 PM
sepavloff accepted this revision.Jul 13 2020, 9:53 PM

LGTM

I would also make commit title a bit shorter, for example [llvm][unittests] Fix ProgramEnvTest.TestExecuteAndWaitStatistics on Solaris.

This revision is now accepted and ready to land.Jul 13 2020, 9:53 PM
Closed by commit rG2d1fe0c37247: [llvm][unittests] Fix ProgramEnvTest.TestExecuteAndWaitStatistics on Solaris (authored by ro). · Explain WhyJul 14 2020, 2:30 AM
This revision was automatically updated to reflect the committed changes.
ro marked an inline comment as done.
ro added a comment.Jul 14 2020, 2:34 AM
In D83661#2149163, @sepavloff wrote:

I would also make commit title a bit shorter, for example [llvm][unittests] Fix ProgramEnvTest.TestExecuteAndWaitStatistics on Solaris.

Sure, fine with me. It were nice if lit would generate unique and reasonably short test names out of the box :-)

Revision Contents

PathSize
llvm/
unittests/
Support/
1 line

Diff 277719

llvm/unittests/Support/ProgramTest.cpp

Show First 20 Lines • Show All 350 Lines • ▼ Show 20 LinesTEST_F(ProgramEnvTest, TestExecuteAndWaitStatistics) {
addEnvVar("LLVM_PROGRAM_TEST_STATISTICS=1"); addEnvVar("LLVM_PROGRAM_TEST_STATISTICS=1");
std::string Error; std::string Error;
bool ExecutionFailed; bool ExecutionFailed;
Optional<ProcessStatistics> ProcStat; Optional<ProcessStatistics> ProcStat;
int RetCode = ExecuteAndWait(Executable, argv, getEnviron(), {}, 0, 0, &Error, int RetCode = ExecuteAndWait(Executable, argv, getEnviron(), {}, 0, 0, &Error,
&ExecutionFailed, &ProcStat); &ExecutionFailed, &ProcStat);
ASSERT_EQ(0, RetCode); ASSERT_EQ(0, RetCode);
ASSERT_TRUE(ProcStat); ASSERT_TRUE(ProcStat);
ASSERT_GT(ProcStat->PeakMemory, 0U);
ASSERT_GE(ProcStat->UserTime, std::chrono::microseconds(0)); ASSERT_GE(ProcStat->UserTime, std::chrono::microseconds(0));
sepavloffUnsubmitted
Done
ReplyInline Actions

PeakMemory is uint64_t, so the check condition is always true. It may cause compiler warnings.

I would propose to remove this check at all.

sepavloff: `PeakMemory` is `uint64_t`, so the check condition is always true. It may cause compiler…
roAuthorUnsubmitted
Done
ReplyInline Actions

On Solaris (and the BSDs where getrusage originated), it's
long instead, thus no warning.

But I agree: the check is pretty pointless now.

ro: On Solaris (and the BSDs where `getrusage` originated), it's `long` instead, thus no warning.
ASSERT_GE(ProcStat->TotalTime, ProcStat->UserTime); ASSERT_GE(ProcStat->TotalTime, ProcStat->UserTime);
} }
} // end anonymous namespace } // end anonymous namespace