-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Conversation
@jkotas |
@@ -190,6 +190,11 @@ struct DT_RTL_USER_PROCESS_PARAMETERS | |||
|
|||
#endif // !FEATURE_PAL | |||
|
|||
// TODO-ARM64-NYI Support for SOS on target with 64K pages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue #10982
src/inc/utilcode.h
Outdated
@@ -1490,6 +1490,15 @@ class CPUGroupInfo | |||
int GetCurrentProcessCpuCount(); | |||
DWORD_PTR GetCurrentProcessCpuMask(); | |||
|
|||
size_t GetOsPageSize(); | |||
|
|||
#define PAGE_SIZE GetOsPageSize() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Windows build still expects this to be a constant. I will put this in an #ifdef FEATURE_PAL block.
b3430c4
to
8391b34
Compare
@@ -86,6 +86,10 @@ bool GCToOSInterface::Initialize() | |||
|
|||
g_logicalCpuCount = cpuCount; | |||
|
|||
assert(g_helperPage == 0); | |||
|
|||
g_helperPage = static_cast<uint8_t*>(mmap(0, OS_PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return false
if this fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
src/gc/unix/gcenv.unix.cpp
Outdated
// Get size of an OS memory page | ||
void GCToOSInterface::GetPageSize() | ||
{ | ||
static pageSize = (sysconf( _SC_PAGE_SIZE ) != -1) ? sysconf( _SC_PAGE_SIZE ) : 4096; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe function local static constructors are a problematic construct. They are not always thread safe, have varying perf characteristics, ... .
I think this should be rather initialize in GCToOSInterface::Inititialize
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it would be nice to call sysconf( _SC_PAGE_SIZE )
just once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the STL in this file - std::call_once
is probably safer and more standard. Though +1 for it going into GCToOSInterface::Initialize
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
src/pal/src/thread/process.cpp
Outdated
@@ -3001,6 +3001,8 @@ Return | |||
BOOL | |||
InitializeFlushProcessWriteBuffers() | |||
{ | |||
s_helperPage = static_cast<int*>(mmap(0, VIRTUAL_PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check for null.
src/gc/unix/gcenv.unix.cpp
Outdated
@@ -517,6 +523,14 @@ void GCToOSInterface::GetMemoryStatus(uint32_t* memory_load, uint64_t* available | |||
*available_page_file = 0; | |||
} | |||
|
|||
// Get size of an OS memory page | |||
void GCToOSInterface::GetPageSize() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering whether this needs to be in .inl file to get inlined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With move to GCToOSInterface::Inititialize, this will be moved to header and be inlined.
src/gc/gc.cpp
Outdated
@@ -26608,7 +26608,7 @@ void gc_heap::revisit_written_pages (BOOL concurrent_p, BOOL reset_only_p) | |||
card_table [card_word (card_of (background_written_addresses [i]))] = ~0u; | |||
dprintf (3,("Set Cards [%p:%p, %p:%p[", | |||
card_of (background_written_addresses [i]), g_addresses [i], | |||
card_of (background_written_addresses [i]+OS_PAGE_SIZE), background_written_addresses [i]+OS_PAGE_SIZE)); | |||
card_of (background_written_addresses [i]+GC_PAGE_SIZE), background_written_addresses [i]+GC_PAGE_SIZE)); | |||
#endif //NO_WRITE_BARRIER | |||
uint8_t* page = (uint8_t*)background_written_addresses[i]; | |||
dprintf (3, ("looking at page %d at %Ix(h: %Ix)", i, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there may need to be more places to flip from OS_PAGE_SIZE
to GC_PAGE_SIZE
in the GC, for performance reasons. Could you please open an issue on it under area-GC
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue #11017
src/gc/gcpriv.h
Outdated
@@ -4302,16 +4302,22 @@ dynamic_data* gc_heap::dynamic_data_of (int gen_number) | |||
return &dynamic_data_table [ gen_number ]; | |||
} | |||
|
|||
#ifdef FEATURE_PAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be under FEATURE_PAL
. We want to avoid having FEATURE_PAL
in the GC implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
src/gc/unix/gcenv.unix.cpp
Outdated
// Get size of an OS memory page | ||
void GCToOSInterface::GetPageSize() | ||
{ | ||
static pageSize = (sysconf( _SC_PAGE_SIZE ) != -1) ? sysconf( _SC_PAGE_SIZE ) : 4096; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the STL in this file - std::call_once
is probably safer and more standard. Though +1 for it going into GCToOSInterface::Initialize
instead.
src/gc/windows/gcenv.windows.cpp
Outdated
// Get size of an OS memory page | ||
void GCToOSInterface::GetPageSize() | ||
{ | ||
return 4096; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be nice to assert that g_SystemInfo.dwPageSize
equals 4096 here, our handling of OS write watch is going to be subtly wrong otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
The Windows failures appear to be unrelated. The CentOS7.1, FreeBSD, OSX10.12, Ubuntu x64 Checked failures are due to trying to compile softwarewritewatch.cpp, which is intended to be a windows only file. My best guess is this is due to https://github.com/dotnet/coreclr/blob/master/src/gc/sample/CMakeLists.txt#L21 |
softwarewritewatch.cpp is not windows only file. It is AMD64 Unix file only actually. Everything in the file is under |
@jkotas Thanks for clarifying. I saw it under if(NOT CLR_CMAKE_PLATFORM_UNIX) in gc/CMakeList.txt and assumed it was only for Windows. |
src/gc/gc.cpp
Outdated
@@ -26608,7 +26608,7 @@ void gc_heap::revisit_written_pages (BOOL concurrent_p, BOOL reset_only_p) | |||
card_table [card_word (card_of (background_written_addresses [i]))] = ~0u; | |||
dprintf (3,("Set Cards [%p:%p, %p:%p[", | |||
card_of (background_written_addresses [i]), g_addresses [i], | |||
card_of (background_written_addresses [i]+OS_PAGE_SIZE), background_written_addresses [i]+OS_PAGE_SIZE)); | |||
card_of (background_written_addresses [i]+GC_PAGE_SIZE), background_written_addresses [i]+GC_PAGE_SIZE)); | |||
#endif //NO_WRITE_BARRIER | |||
uint8_t* page = (uint8_t*)background_written_addresses[i]; | |||
dprintf (3, ("looking at page %d at %Ix(h: %Ix)", i, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue #11017
src/gc/gc.cpp
Outdated
@@ -26870,7 +26870,7 @@ BOOL gc_heap::create_bgc_thread_support() | |||
} | |||
|
|||
//needs to have room for enough smallest objects fitting on a page | |||
parr = new (nothrow) (uint8_t* [1 + page_size / MIN_OBJECT_SIZE]); | |||
parr = new (nothrow) uint8_t*[1 + page_size / MIN_OBJECT_SIZE]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
#define page_size OS_PAGE_SIZE
src/gc/gcpriv.h
Outdated
@@ -4302,16 +4302,22 @@ dynamic_data* gc_heap::dynamic_data_of (int gen_number) | |||
return &dynamic_data_table [ gen_number ]; | |||
} | |||
|
|||
#ifdef FEATURE_PAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
@@ -86,6 +86,10 @@ bool GCToOSInterface::Initialize() | |||
|
|||
g_logicalCpuCount = cpuCount; | |||
|
|||
assert(g_helperPage == 0); | |||
|
|||
g_helperPage = static_cast<uint8_t*>(mmap(0, OS_PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
src/gc/unix/gcenv.unix.cpp
Outdated
@@ -517,6 +523,14 @@ void GCToOSInterface::GetMemoryStatus(uint32_t* memory_load, uint64_t* available | |||
*available_page_file = 0; | |||
} | |||
|
|||
// Get size of an OS memory page | |||
void GCToOSInterface::GetPageSize() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With move to GCToOSInterface::Inititialize, this will be moved to header and be inlined.
src/gc/unix/gcenv.unix.cpp
Outdated
// Get size of an OS memory page | ||
void GCToOSInterface::GetPageSize() | ||
{ | ||
static pageSize = (sysconf( _SC_PAGE_SIZE ) != -1) ? sysconf( _SC_PAGE_SIZE ) : 4096; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
src/gc/windows/gcenv.windows.cpp
Outdated
// Get size of an OS memory page | ||
void GCToOSInterface::GetPageSize() | ||
{ | ||
return 4096; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
src/utilcode/util.cpp
Outdated
size_t GetOsPageSize() | ||
{ | ||
#ifdef FEATURE_PAL | ||
static size_t pageSize = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to deal with static here too
BTW: I have opened https://github.com/dotnet/coreclr/issues/11018 to track enabling |
src/utilcode/util.cpp
Outdated
// | ||
// However there is a real potential performance cost. This will need to | ||
// be optimized if GetOsPageSize() shows up in profiles. | ||
static const size_t pageSize(GetOsPageSizeUncached()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkotas This is not intended to ignore your request. Per @swgillespie 's suggestion I looked at the docs for std::call_once here. http://en.cppreference.com/w/cpp/thread/call_once. It was noted that the standard makes
the guarantee noted in 1327-1328. (Needs citation). I recognize the potential performance impact you noted, but I do not know a good/simple way to guarantee this is initialized without using the function-local static guarantee.
src/utilcode/util.cpp
Outdated
// | ||
// However there is a real potential performance cost. This will need to | ||
// be optimized if GetOsPageSize() shows up in profiles. | ||
static const size_t pageSize(GetOsPageSizeUncached()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkotas It was not obvious how to play the same initialization trick for util.cpp. Therefore I looked at @swgillespie 's call_once suggestion which suggested function-local statics are preferred over call_once.
Based on my understanding of the function-local static requirements the above code is guaranteed to be functionally correct. But as you noted performance may be an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use a global Volatile<size_t> s_pageSize;
. In this function, you would read it, compare to zero and if it is zero, call the GetOsPageSizeUncached()
and assign the result to s_pageSize. Without any locks. I think that you could even use s_pageSize.LoadWithoutBarrier / s_pageSize.StoreWithoutBarrier. The only effect would be that on multiple processors, we could end up calling the GetOsPageSizeUncached few more times.
Looks like windows errors are related after all. gcwks.cpp is failing to compile with a signed/unsigned mismatch. |
Windows errors are due to "unresolved external symbol "protected: static unsigned int GCToOSInterface::s_pageSize"" |
src/gc/unix/gcenv.unix.cpp
Outdated
@@ -471,7 +486,7 @@ uint64_t GCToOSInterface::GetPhysicalMemoryLimit() | |||
long pages = sysconf(_SC_PHYS_PAGES); | |||
if (pages == -1) | |||
{ | |||
return 0; | |||
return 0;SC_PAGE_SIZE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accidental change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkotas Yeah. My editor went haywire writing random clipboard strings throughout files. I corrected a few dozen, but this one must have slipped through.
src/gc/env/gcenv.os.h
Outdated
@@ -128,6 +128,9 @@ typedef void (*GCThreadFunction)(void* param); | |||
// Interface that the GC uses to invoke OS specific functionality | |||
class GCToOSInterface | |||
{ | |||
protected: | |||
static size_t s_pageSize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer keeping it here as a pure static interface without any static data members and implementations of methods. Could you please rather create a g_pageSize variable in the src/gc/unix/gcenv.unix.cpp? The Windows implementation of the GetPageSize (in src/gc/windows/gcenv.windows.cpp) can directly return g_SystemInfo.dwPageSize.
Btw, you were missing the s_pageSize setting in the src/vm/gcenv.os.cpp, this is the interface implementation for CoreCLR with GC embedded in the coreclr shared library, which is the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to add gcenv.windows.inl
and gcenv.unix.inl
files to allow inlining of the OS specific implementation details.
I think we will likely end up with number of these perf critical cases where inlining is important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkotas that makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which files would include the new inl files.
gcenv.h
How would they determine which one to include?
Ifdefs.
src/gc/env/gcenv.base.h
Outdated
@@ -300,7 +300,7 @@ typedef DPTR(uint8_t) PTR_uint8_t; | |||
|
|||
#define DECLSPEC_ALIGN(x) __declspec(align(x)) | |||
|
|||
#define OS_PAGE_SIZE 4096 | |||
#define OS_PAGE_SIZE GCToOSInterface::GetPageSize() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer replacing all usages of OS_PAGE_SIZE by the GCToOSInterface::GetPageSize() instead. Seeing a macro in the source gives me a false feeling that it is a constant. I would also prefer the same for the PAGE_SIZE.
src/utilcode/util.cpp
Outdated
// | ||
// However there is a real potential performance cost. This will need to | ||
// be optimized if GetOsPageSize() shows up in profiles. | ||
static const size_t pageSize(GetOsPageSizeUncached()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use a global Volatile<size_t> s_pageSize;
. In this function, you would read it, compare to zero and if it is zero, call the GetOsPageSizeUncached()
and assign the result to s_pageSize. Without any locks. I think that you could even use s_pageSize.LoadWithoutBarrier / s_pageSize.StoreWithoutBarrier. The only effect would be that on multiple processors, we could end up calling the GetOsPageSizeUncached few more times.
src/pal/src/include/pal/virtual.h
Outdated
BOUNDARY_64K = 0xffff | ||
}; | ||
|
||
#define VIRTUAL_PAGE_SIZE getpagesize() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also replace the VIRTUAL_PAGE_SIZE macro usages with a function call (e.g. GetVirtualPageSize) and cache the getpagesize call.
All of the VIRTUAL_PAGE_MASK usages are for alignment purposes and using ALIGN_UP / ALIGN_DOWN / IS_ALIGNED with the virtual page size as the parameter would eliminate the need for this macro.
@janvorli @jkotas @swgillespie @Maoni0 @adityamandaleeka I have attempted to address all of the review feedback. |
Windows build failure is
I will address tomorrow. |
src/debug/createdump/crashinfo.cpp
Outdated
@@ -493,11 +493,11 @@ void | |||
CrashInfo::InsertMemoryRegion(uint64_t address, size_t size) | |||
{ | |||
// Round to page boundary | |||
uint64_t start = address & PAGE_MASK; | |||
uint64_t start = ROUND_DOWN_TO_PAGE(address); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janvorli The changes to the files in this directory are causing the Ubuntu x64 failures. I had removed all references to PAGE_SIZE and PAGE_MASK from the source tree. Since these ROUND_*_TO_PAGE macros were defined next to them I assumed they were safe to use....
In any case these macros must be coming indirectly from the host include files.
Reverting all the changes in this directory allows Ubuntu compile.
My plan is to add these to the list of fixes in the SOS debugger issue.
I just got back from vacation - I'll take a look at this tomorrow. |
4 builds timed out. Trying again @dotnet-bot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The timeouts have apparently recurred. I have reviewed the code and do not see huge differences which would explain the timeouts. I will fix all the functional changes I observed while reviewing the code.
It is not obvious to me that this will fix the timeouts. If anyone sees something I am missing. Please let me know.
src/pal/src/map/virtual.cpp
Outdated
@@ -885,10 +889,8 @@ static LPVOID VIRTUALReserveMemory( | |||
// First, figure out where we're trying to reserve the memory and | |||
// how much we need. On most systems, requests to mmap must be | |||
// page-aligned and at multiples of the page size. | |||
StartBoundary = (UINT_PTR)lpAddress & ~BOUNDARY_64K; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a functional change on 4K systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to stay to be compatible with Windows. I am not sure, but I think there was some code in the VM or JIT that relies on that.
MemSize = ( ((UINT_PTR)lpAddress + dwSize + VIRTUAL_PAGE_MASK) & ~VIRTUAL_PAGE_MASK ) - | ||
StartBoundary; | ||
StartBoundary = (UINT_PTR) ALIGN_DOWN(lpAddress, GetVirtualPageSize()); | ||
MemSize = ALIGN_UP((UINT_PTR)lpAddress + dwSize, GetVirtualPageSize()) - StartBoundary; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is not quite functionally identical. On first glance looks like off by one. Also all similar lines in this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I understand why would it be functionally different.
The ALIGN_UP
does result = (val + (alignment - 1)) & ~(alignment - 1);
The ALIGN_DOWN
does result = val & ~(alignment - 1);
So ALIGN_DOWN(lpAddress, GetVirtualPageSize())
gets lpAddress & ~(GetVirtualPageSize() - 1)
And ALIGN_UP((UINT_PTR)lpAddress + dwSize, GetVirtualPageSize())
gets (lpAddress + dwSize + (GetVirtualPageSize() - 1)) & ~(GetVirtualPageSize() - 1)
Using the previous state and replacing VIRTUAL_PAGE_MASK by (GetVirtualPageSize() - 1) yields the same expressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janvorli The original expression was missing a -1
So to match exactly.... The code needs to be
ALIGN_UP((UINT_PTR)lpAddress + dwSize + 1, GetVirtualPageSize()) - StartBoundary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janvorli I will try restoring the 64K boundary first as it seem more likely the cause of the regression
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still not sure I understand where was the missing -1. The original expression was
MemSize = ( ((UINT_PTR)lpAddress + dwSize + VIRTUAL_PAGE_MASK) & ~VIRTUAL_PAGE_MASK ) - StartBoundary;
VIRTUAL_PAGE_MASK is PAGE_SIZE - 1
So just substituting (GetVirtualPageSize() - 1) for VIRTUAL_PAGE_MASK:
MemSize = ( ((UINT_PTR)lpAddress + dwSize + (GetVirtualPageSize() - 1)) & ~(GetVirtualPageSize() - 1)) - StartBoundary;
And your new expression is
MemSize = ALIGN_UP((UINT_PTR)lpAddress + dwSize, GetVirtualPageSize()) - StartBoundary;
Expanding the macro yields
MemSize = (((UINT_PTR)lpAddress + dwSize + (GetVirtualPageSize() - 1)) & ~(GetVirtualPageSize() - 1)) - StartBoundary;
Which is exactly the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janvorli Yes you are right. I had been reading the first VIRTUAL_PAGE_MASK as VIRTUAL_PAGE_SIZE. Sometimes you are so blind, you cannot see something even if someone shows you. Thanks for insisting..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sdmaclea no problem, similar things happen to me quite often too.
@dotnet-bot help |
Welcome to the dotnet/coreclr Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
Failing Ubuntu test are PAL tests. I will try to reproduce on Ubuntu x64 16.04. |
src/pal/src/debug/debug.cpp
Outdated
@@ -609,7 +610,7 @@ PAL_ProbeMemory( | |||
} | |||
|
|||
// Round to the beginning of the next page | |||
pBuffer = (PVOID)(((SIZE_T)pBuffer & ~VIRTUAL_PAGE_MASK) + VIRTUAL_PAGE_SIZE); | |||
pBuffer = ALIGN_UP(pBuffer, GetVirtualPageSize()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the bug.
@janvorli paltest_pal_sxs_test1 is failing on CentOS7.1 x64 Debug and FreeBSD x64. The problem does not reproduce on Ubuntu 16.04 Debug or Checked. Can you take a look? |
@sdmaclea the windows jobs are publicly defined but can only be launched by whitelisted users. |
I made another full arm64 Ubuntu run. This time it looks like it completed successfully. After 15h there was an entry in the log
It took many more hours for the running tests to finish. And the logs ended with
Regarding
The last several pages of the first runs logs did not start any new tests. Therefore I will assert that the failure occurred > 15h into the run while it was in a shutdown phase. |
Yeah, that's no good - I noticed this too the last time I tried to run this job. I'll see if I can get that fixed soon. |
@dotnet-bot |
Please remove the "No-Merge" label. I believe this can and should be merged. Retriggered jobs which never completed due to a Jenkins restart while they were triggered. |
9f29d53
to
0fee73a
Compare
I just rebased to tip, squashed and forced update. No code changes. I just wanted it to be easier for final review. |
There are seg faults on the tip. The order of operations during startup has changed since this code was written. The @jkotas How do you want me to handle it?
|
I think that the call to |
+1 to that, in particular I think it makes sense for it to go after |
OK. I moved it as suggested. It fixed it on arm64. Presumably it will fix it for x64 & arm/armel as well. |
Now that all tests are passing again. Can this be merged? |
Ping |
sorry, I can't tell if you were able to run gc_reliability_framework on arm64. Could you please clarify? |
@Maoni0 I was able to successfully run it once on [Arm64/Unix]. It took 28 hrs to run. (I manually ran it on a 64K page system). |
ok great. I'll merge it. thanks for all the work! |
Thanks. It is a relief to have this merged. |
* [Arm64/Unix] Support 64K pages * GC move GCToOSInterface::Initialize() into InitializeGarbageCollector()
:mips-interest |
Excludes fixes for crossgen which are being addressed by #10959.
Excludes fix for SOS. An separate issue will be created.