From 5946a2cd15e7b2a301ffa906cbc82165d558fa2d Mon Sep 17 00:00:00 2001 From: "Zdunowski, Piotr" Date: Tue, 3 Apr 2018 16:06:37 +0200 Subject: [PATCH] Fix printf issue with printing pointers from 32bit kernel on 64bit system. Change-Id: I77771b4ebe6c4335d51dc1834f0b8f9df2a069a4 --- runtime/kernel/kernel.h | 4 +++ runtime/os_interface/linux/print.cpp | 6 +---- runtime/os_interface/windows/print.cpp | 6 +---- runtime/program/print_formatter.cpp | 30 ++++++++++++++++++---- runtime/program/print_formatter.h | 21 +++++---------- unit_tests/kernel/kernel_tests.cpp | 24 +++++++++++++++++ unit_tests/program/printf_helper_tests.cpp | 29 ++++++++++++++++++++- 7 files changed, 89 insertions(+), 31 deletions(-) diff --git a/runtime/kernel/kernel.h b/runtime/kernel/kernel.h index cb51cf848b..098e59cecd 100644 --- a/runtime/kernel/kernel.h +++ b/runtime/kernel/kernel.h @@ -358,6 +358,10 @@ class Kernel : public BaseObject<_cl_kernel> { return isParentKernel && getProgram()->getBlockKernelManager()->getIfBlockUsesPrintf(); } + bool is32Bit() const { + return kernelInfo.gpuPointerSize == 4; + } + int32_t getDebugSurfaceBti() const { if (kernelInfo.patchInfo.pAllocateSystemThreadSurface) { return kernelInfo.patchInfo.pAllocateSystemThreadSurface->BTI; diff --git a/runtime/os_interface/linux/print.cpp b/runtime/os_interface/linux/print.cpp index bf417e3763..1a45f87b02 100644 --- a/runtime/os_interface/linux/print.cpp +++ b/runtime/os_interface/linux/print.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2017, Intel Corporation + * Copyright (c) 2017 - 2018, Intel Corporation * * Permission is hereby granted, free of charge, to any person obtaining a * copy of this software and associated documentation files (the "Software"), @@ -39,10 +39,6 @@ size_t simple_sprintf(char *output, size_t outputSize, const char *format, const return snprintf(output, outputSize, format, value); } -size_t simple_sprintf(char *output, size_t outputSize, const char *format, void *value) { - return snprintf(output, outputSize, format, value); -} - template size_t simple_sprintf(char *output, size_t output_size, const char *format, float value); template size_t simple_sprintf(char *output, size_t output_size, const char *format, double value); template size_t simple_sprintf(char *output, size_t output_size, const char *format, char value); diff --git a/runtime/os_interface/windows/print.cpp b/runtime/os_interface/windows/print.cpp index a17c9e8cd2..152384021b 100644 --- a/runtime/os_interface/windows/print.cpp +++ b/runtime/os_interface/windows/print.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2017, Intel Corporation + * Copyright (c) 2017 - 2018, Intel Corporation * * Permission is hereby granted, free of charge, to any person obtaining a * copy of this software and associated documentation files (the "Software"), @@ -80,10 +80,6 @@ size_t simple_sprintf(char *output, size_t outputSize, const char *format, const return sprintf_s(output, outputSize, format, value); } -size_t simple_sprintf(char *output, size_t outputSize, const char *format, void *value) { - return sprintf_s(output, outputSize, format, value); -} - template size_t simple_sprintf(char *output, size_t output_size, const char *format, float value); template size_t simple_sprintf(char *output, size_t output_size, const char *format, double value); template size_t simple_sprintf(char *output, size_t output_size, const char *format, char value); diff --git a/runtime/program/print_formatter.cpp b/runtime/program/print_formatter.cpp index a676cbffde..7fedbde082 100644 --- a/runtime/program/print_formatter.cpp +++ b/runtime/program/print_formatter.cpp @@ -119,7 +119,6 @@ void PrintFormatter::stripVectorTypeConversion(char *format) { size_t PrintFormatter::printToken(char *output, size_t size, const char *formatString) { PRINTF_DATA_TYPE type(PRINTF_DATA_TYPE::INVALID); - size_t ret = 0; read(&type); switch (type) { @@ -134,10 +133,7 @@ size_t PrintFormatter::printToken(char *output, size_t size, const char *formatS case PRINTF_DATA_TYPE::LONG: return typedPrintToken(output, size, formatString); case PRINTF_DATA_TYPE::POINTER: - ret = typedPrintToken(output, size, formatString); - // always pad read data to 8 bytes when handling pointers - offset += 8 - sizeof(void *); - return ret; + return printPointerToken(output, size, formatString); case PRINTF_DATA_TYPE::DOUBLE: return typedPrintToken(output, size, formatString); case PRINTF_DATA_TYPE::VECTOR_BYTE: @@ -157,6 +153,30 @@ size_t PrintFormatter::printToken(char *output, size_t size, const char *formatS } } +size_t PrintFormatter::printStringToken(char *output, size_t size, const char *formatString) { + int index = 0; + int type = 0; + // additional read to discard the token + read(&type); + read(&index); + if (type == static_cast(PRINTF_DATA_TYPE::STRING)) { + return simple_sprintf(output, size, formatString, kernel.getKernelInfo().queryPrintfString(index)); + } else { + return simple_sprintf(output, size, formatString, 0); + } +} + +size_t PrintFormatter::printPointerToken(char *output, size_t size, const char *formatString) { + uint64_t value = {0}; + read(&value); + + if (kernel.is32Bit()) { + value &= 0x00000000FFFFFFFF; + } + + return simple_sprintf(output, size, formatString, value); +} + char PrintFormatter::escapeChar(char escape) { switch (escape) { case 'n': diff --git a/runtime/program/print_formatter.h b/runtime/program/print_formatter.h index d744252ebf..12432cd161 100644 --- a/runtime/program/print_formatter.h +++ b/runtime/program/print_formatter.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2017, Intel Corporation + * Copyright (c) 2017 - 2018, Intel Corporation * * Permission is hereby granted, free of charge, to any person obtaining a * copy of this software and associated documentation files (the "Software"), @@ -63,6 +63,8 @@ class PrintFormatter { protected: void printString(const char *formatString, const std::function &print); size_t printToken(char *output, size_t size, const char *formatString); + size_t printStringToken(char *output, size_t size, const char *formatString); + size_t printPointerToken(char *output, size_t size, const char *formatString); char escapeChar(char escape); bool isConversionSpecifier(char c); @@ -108,8 +110,9 @@ class PrintFormatter { for (int i = 0; i < valueCount; i++) { read(&value); charactersPrinted += simple_sprintf(output + charactersPrinted, size - charactersPrinted, strippedFormat, value); - if (i < valueCount - 1) + if (i < valueCount - 1) { charactersPrinted += simple_sprintf(output + charactersPrinted, size - charactersPrinted, "%c", ','); + } } if (sizeof(T) < 4) { @@ -119,18 +122,6 @@ class PrintFormatter { return charactersPrinted; } - size_t printStringToken(char *output, size_t size, const char *formatString) { - int index = 0; - int type = 0; - // additional read to discard the data type - read(&type); - read(&index); - if (type == static_cast(PRINTF_DATA_TYPE::STRING)) - return simple_sprintf(output, size, formatString, kernel.getKernelInfo().queryPrintfString(index)); - else - return simple_sprintf(output, size, formatString, 0); - } - Kernel &kernel; GraphicsAllocation &data; @@ -138,4 +129,4 @@ class PrintFormatter { uint32_t bufferSize; // size of the data contained in the buffer uint32_t offset; // current position in currently parsed buffer }; -}; +}; // namespace OCLRT diff --git a/unit_tests/kernel/kernel_tests.cpp b/unit_tests/kernel/kernel_tests.cpp index 92c28351fc..d9ce885985 100644 --- a/unit_tests/kernel/kernel_tests.cpp +++ b/unit_tests/kernel/kernel_tests.cpp @@ -2128,3 +2128,27 @@ TEST(KernelTest, givenKernelWhenDebugFlagToUseMaxSimdForCalculationsIsUsedThenMa kernel.mockKernel->getWorkGroupInfo(device.get(), CL_KERNEL_WORK_GROUP_SIZE, sizeof(size_t), &maxKernelWkgSize, nullptr); EXPECT_EQ(256u, maxKernelWkgSize); } + +TEST(KernelTest, givenKernelWithKernelInfoWith32bitPointerSizeThenReport32bit) { + KernelInfo info; + info.gpuPointerSize = 4; + + MockContext context; + MockProgram program(&context, false); + std::unique_ptr device(Device::create(nullptr)); + std::unique_ptr kernel(new MockKernel(&program, info, *device.get())); + + EXPECT_TRUE(kernel->is32Bit()); +} + +TEST(KernelTest, givenKernelWithKernelInfoWith64bitPointerSizeThenReport64bit) { + KernelInfo info; + info.gpuPointerSize = 8; + + MockContext context; + MockProgram program(&context, false); + std::unique_ptr device(Device::create(nullptr)); + std::unique_ptr kernel(new MockKernel(&program, info, *device.get())); + + EXPECT_FALSE(kernel->is32Bit()); +} diff --git a/unit_tests/program/printf_helper_tests.cpp b/unit_tests/program/printf_helper_tests.cpp index 0e39edd2a3..ff1a2a3af6 100644 --- a/unit_tests/program/printf_helper_tests.cpp +++ b/unit_tests/program/printf_helper_tests.cpp @@ -749,7 +749,7 @@ TEST_F(PrintFormatterTest, GivenPrintfFormatWhenPointerThenInsertAddress) { storeData(reinterpret_cast(&temp)); // on 32bit configurations add extra 4 bytes when storing pointers, IGC always stores pointers on 8 bytes - if (!is64bit) { + if (is32bit) { uint32_t padding = 0; storeData(padding); } @@ -764,6 +764,33 @@ TEST_F(PrintFormatterTest, GivenPrintfFormatWhenPointerThenInsertAddress) { EXPECT_STREQ(referenceOutput, actualOutput); } +TEST_F(PrintFormatterTest, GivenPrintfFormatWhenPointerWith32BitKernelThenPrint32BitPointer) { + auto stringIndex = injectFormatString("%p"); + storeData(stringIndex); + kernelInfo->gpuPointerSize = 4; + + storeData(PRINTF_DATA_TYPE::POINTER); + + // store pointer + uint32_t addressValue = 0; + storeData(addressValue); + + void *pointer = nullptr; + + // store non zero padding + uint32_t padding = 0xdeadbeef; + storeData(padding); + + char actualOutput[PrintFormatter::maxPrintfOutputLength]; + char referenceOutput[PrintFormatter::maxPrintfOutputLength]; + + snprintf(referenceOutput, sizeof(referenceOutput), "%p", pointer); + + printFormatter->printKernelOutput([&actualOutput](char *str) { strncpy_s(actualOutput, PrintFormatter::maxPrintfOutputLength, str, PrintFormatter::maxPrintfOutputLength); }); + + EXPECT_STREQ(referenceOutput, actualOutput); +} + TEST_F(PrintFormatterTest, GivenPrintfFormatWhen2ByteVectorsThenParseDataBufferProperly) { int channelCount = 4;