-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
Refactor: handle in-output errors for string functions (Part 2) #12064
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for meta-velox canceled.
|
velox/tpch/gen/dbgen/CMakeLists.txt
Outdated
@@ -30,6 +30,6 @@ add_library( | |||
|
|||
target_compile_definitions(dbgen PRIVATE DBNAME=dss MAC ORACLE TPCH) | |||
target_include_directories(dbgen PRIVATE include) | |||
|
|||
target_link_libraries(dbgen PUBLIC Folly::folly) |
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 be INTERFACE instead of PUBLIC. We don;t actually need to link to Folly here. We only want it to get to the header for the macro FOLLY_UNLIKELY.
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.
Sorry that's actually not true, I was unsure and made a little test, see below.
The a folly header is included in a header but also an implementation file so we need to link as PUBLIC
to make folly's include dir available both to targets depending on this target (aka anyone using this header) and when building build.cpp
.
INTERFACE
will not add folly's include dir to the compiler invocation, only to the one of targets:
cmake_minimum_required(VERSION 3.20)
project(test)
set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
add_library(main a.cpp)
add_library(lib_a INTERFACE)
target_include_directories(lib_a INTERFACE ${CMAKE_CURRENT_LIST_DIR}/include)
target_link_libraries(main INTERFACE lib_a)
INTERFACE
[
{
"directory": "~/scratch",
"command": "/usr/bin/c++ -o CMakeFiles/main.dir/a.cpp.o -c ~/scratch/a.cpp",
"file": "~/scratch/a.cpp",
"output": "CMakeFiles/main.dir/a.cpp.o"
}
]
PUBLIC
[
{
"directory": "~/scratch",
"command": "/usr/bin/c++ -I~/scratch/include -o CMakeFiles/main.dir/a.cpp.o -c ~/scratch/a.cpp",
"file": "~/scratch/a.cpp",
"output": "CMakeFiles/main.dir/a.cpp.o"
}
]
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.
reverted
#define PR_DATE(tgt, yr, mn, dy) \ | ||
do { \ | ||
int res = sprintf(tgt, "19%02ld-%02ld-%02ld", yr, mn, dy); \ | ||
if(FOLLY_UNLIKELY(res < 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.
How come the header for this macro include isn't needed?
It seems to have been needed in the build.cpp file.
How come? Is it not actually needed everywhere because it comes in from some other include?
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.
It might be coming from some other file, I will include it explicitly.
velox/tpch/gen/dbgen/build.cpp
Outdated
sprintf(target + 3, "%03d", static_cast<int>(acode)); | ||
sprintf(target + 7, "%03d", static_cast<int>(exchg)); | ||
sprintf(target + 11, "%04d", static_cast<int>(number)); | ||
int res = sprintf(target, "%02d", static_cast<int>(10 + (ind % NATIONS_MAX))); |
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.
Why not use fmt::format instead of the complications with sprintf ?
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.
Done, but still we need to convert std::string back to c style right and store the output in target.
#define PR_DATE(tgt, yr, mn, dy) sprintf(tgt, "%02d-%02d-19%02d", mn, dy, yr) | ||
#define PR_DATE(tgt, yr, mn, dy) \ | ||
do { \ | ||
int res = sprintf(tgt, "19%02ld-%02ld-%02ld", yr, mn, dy); \ |
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.
fmt::format would be simpler here as well.
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.
One doubt though, fmt::format returns a cpp string whereas tgt is supposed to be a c style null terminated string(char array) . So do you want me to use fmt::format followed by strncpy as in :
#define PR_DATE(tgt, yr, mn, dy) \
do { \
std::string formatted = fmt::format("{:02d}-{:02d}-19{:02d}", mn, dy, yr); \
std::strncpy(tgt, formatted.c_str(), formatted.size() + 1); \
tgt[formatted.size() + 1] = '\0'
} while (0)
PR Description:
Detect and address input/output errors that can result in undefined behavior. Ensure proper error handling for I/O functions that may fail and leave variables uninitialized. Neglecting to check the status of these functions before using their outputs (e.g., memory buffers, file descriptors, etc.) can lead to undefined program behavior. This update enforces checks on commonly used I/O functions to validate their return values and prevent improper usage.
This is the second PR of the several changes for this refactoring.