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
DWARF input format #55450
DWARF input format #55450
Conversation
This is an automated comment for commit abd35e0 with description of existing statuses. It's updated for the latest CI running ✅ Click here to open a full report in a separate page Successful checks
|
ffaf0de
to
782f24d
Compare
150e193
to
a4d4c1c
Compare
e405ad1
to
637822e
Compare
auto tag_names = ColumnString::create(); | ||
/// Note: TagString() returns empty string for tags that don't exist, and tag 0 doesn't exist. | ||
for (uint32_t tag = 0; tag <= UINT16_MAX; ++tag) | ||
append(tag_names, removePrefix(llvm::dwarf::TagString(tag), strlen("DW_TAG_"))); |
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.
we will repeat strlen
for each tag, no? why not store the prefix_size in variable?
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.
Compilers usually optimize it out: https://godbolt.org/z/M7dTqPvKK . Even if not (e.g. in debug build), doing a short strlen 64 K times is cheap enough.
So I thought it's not worth the extra code to avoid this... except I didn't realize that it's only 1 line of extra code. Guess I'll add it, just to prevent the next reader of the code from wondering about this.
if (need[COL_ATTR_FORM] || need[COL_ATTR_INT] || need[COL_ATTR_STR]) | ||
need[COL_ATTR_NAME] = true; | ||
if (need[COL_ANCESTOR_OFFSETS]) | ||
need[COL_ANCESTOR_TAGS] = true; |
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.
can these be a simple assignment?
if (need[COL_ATTR_FORM] || need[COL_ATTR_INT] || need[COL_ATTR_STR]) | |
need[COL_ATTR_NAME] = true; | |
if (need[COL_ANCESTOR_OFFSETS]) | |
need[COL_ANCESTOR_TAGS] = true; | |
need[COL_ATTR_NAME] = need[COL_ATTR_FORM] || need[COL_ATTR_INT] || need[COL_ATTR_STR]; | |
need[COL_ANCESTOR_TAGS] = need[COL_ANCESTOR_OFFSETS]; |
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.
(That would be |=
, which annoyingly produces compiler warnings-treated-as-errors for bool. Will change to x = x || y
, which still looks nicer than the ifs, I guess.)
{ | ||
if (attr.Attr == llvm::dwarf::DW_AT_language) // programming language | ||
append(col_attr_str, removePrefix(llvm::dwarf::LanguageString(static_cast<uint32_t>(val.getRawUValue())), | ||
strlen("DW_LANG_"))); |
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.
does it make sense to create static constexpr std::string_view
for these common prefixes so you can simply call .size()
method on them?
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.
Oh, nice trick, constexpr size_t x = strlen("foo")
doesn't work (lame) but constexpr std::string_view x = "foo"; ... x.size()
does.
Uh oh, MSAN fails with this nonsense:
They can't distinguish between a compiler bug and a program bug! What kind of design is that. Why do I keep running into sanitizer-related compiler bugs. |
@@ -54,9 +54,6 @@ if (SANITIZE) | |||
set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${SAN_FLAGS} ${UBSAN_FLAGS}") | |||
set (CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${SAN_FLAGS} ${UBSAN_FLAGS}") | |||
|
|||
# llvm-tblgen, that is used during LLVM build, doesn't work with UBSan. | |||
set (ENABLE_EMBEDDED_COMPILER 0 CACHE BOOL "") |
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 seemed redundant with the check in contrib/llvm-project-cmake/CMakeLists.txt
, moved the comment into there.
else() | ||
set (ENABLE_EMBEDDED_COMPILER_DEFAULT ON) | ||
set (ENABLE_EMBEDDED_COMPILER_DEFAULT ${ENABLE_LIBRARIES}) |
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.
Made ENABLE_LIBRARIES=0 imply ENABLE_EMBEDDED_COMPILER=0 by default. Hope there isn't a good reason why it wasn't done this way originally.
@@ -1,6 +1,5 @@ | |||
# Quick syntax check (2 minutes on 16-core server) | |||
# Relatively quick syntax check (20 minutes on 16-core server) |
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.
20 minutes is how long it took for me (on master branch without this PR). Did clang get 10x slower in 5 years? :o (More than that because computers got faster. But less because the code got bigger. Let's guess that these two effects roughly cancel out.) I heard that llvm gets slower with each version, but would have expected less than 1.6x/year.
Some possible features for future:
|
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Documentation entry for user-facing changes
Because grepping
dwarfdump
output is very slow and inconvenient.