Isn't phabricator deprecated now, and all new reviews should be done on github?

@dblaikie ping. :) Do you want to continue review here or in github?

OK trying new workflow:
https://github.com/llvm/llvm-project/pull/65516

Also might be easier to explain to an "average" user "Just run llvm-dwarfdump --main-binary=<path_to_main_binary> --debug-info <path_to_dwo>" Vs Dump this DWO, look up DWOID (explain where it is in DWARF4 vs 5), then run llvm-dwarfdump again now with main binary with these flags.

In D159374#4635446, @dblaikie wrote:

Might be easier to go the other way, and when dumping the main binary, also dump associated dwo/dwp files? (Perhaps under a flag, or by default, not sure)

update from upstream branch

rebase

rebase

rebase

Will add tests if the approach is acceptable.

@dblaikie @bulbazord @JDevlieghere WDYT of this approach?
@clayborg One suggestion, maybe move EnableMultiThreading optional paramenter to the end of argument list? That way constructors in other projects don't need to be modified? Right now this will break LLD and BOLT builds.

In D158689#4631799, @weiwang wrote:

In D158689#4629229, @huangjd wrote:

@weiwang Have you tried to generate a flame graph showing how much time each function takes?

I didn't collect this for the measurement. I think this change is pretty isolated to the loader pass, so if there is a win, it would show up in the e2e compilation.

I'll take a few cases and report back.

Tried locally to confirm and build of a module succeeds.
Thank you for fixing it.

@huangjd Do you think you will have time to change implementation to unordered_map as discussed by EOD, otherwise I am reverting the diff to unblock out validation testing.

@huangjd can you follow up and change to unordered_map, so whole diff doesn't have to be reverted?

In D158689#4617692, @davidxl wrote:

The proposal sounds good to me.

In D158689#4617634, @wenlei wrote:

In D158689#4617548, @davidxl wrote:

@wenlei, may I propose having this patch in first (to unblock) @ayermolo while we are discussing longer term strategy? Another benefit is that we can wait to see if there are other incidence (of using long lifetime reference) can show up so that we have more cases to consider the redesign (if needed).

Back-and-forth reverts/relands are costly and somewhat disruptive, I hope we can gain more confidence before reattempting. Here's my suggestion:

Step 1: use HashKeyMap<unordered_map, SampleContext, FunctionSamples> instead of HashKeyMap<DenseMap, SampleContext, FunctionSamples> for SampleProfileMap. It's a one line change to unblock.

Step 2: a separate patch to attempt DenseMap, coupled with all fixes needed. we can also help test correctness and performance on that change. actually the performance picture may not be that clear. reallocation is expensive, and the fact you didn't run into reallocation in your testing may indicate that the perf benchmark result may also be skewed. We also need to take into account the cost to update references etc.

WDYT?

@davidxl While discussion continues can the patch be reverted, or at least changed to unordered_map for now (preferably the former). It has been impacting our internal testing of trunk for a week now.

In D158689#4615531, @huangjd wrote:

DenseMap is significantly faster than std::unordered_map as previously used.

Benchmarking with an internal profile for production, when reading the function offset table alone, the original implementation (before D147740), DenseMap, and unordered_map takes 1.2, 0.78, 0.82 s respectively, and reading the full profiles takes 34.6, 29.0, 31.4 s respectively. That means using a std::unordered_map is 5-8% slower. (Meanwhile the refactoring itself has been very significant regardless of which container to use)

I think https://reviews.llvm.org/D147740 should be reverted and concerns addressed in that patch next re-land.

This does seem to fix memory corruption.
That being said I think I am with @wenlei on this. The implementation seems fragile as DenseMap, apparently, doesn't offer reference stability. Which leads to these kind of workarounds, and more then likely more issues down the road.

In D147740#4611679, @huangjd wrote:

What were you building when this error happened?

In D147740#4608331, @ayermolo wrote:

@huangjd

MSAN output. Does this ring any bells?

==3359553==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x564b193d7f4e in llvm::sampleprof::FunctionSamples::getHeadSamplesEstimate() const /home/ayermolo/local/upstream-llvm/llvm-project/llvm/include/llvm/ProfileData/SampleProf.h:959:30
    #1 0x564b193ce93a in llvm::sampleprof::ProfileConverter::flattenNestedProfile(llvm::sampleprof::SampleProfileMap&, llvm::sampleprof::FunctionSamples const&) /home/ayermolo/local/upstream-llvm/llvm-project/llvm/include/llvm/ProfileData/SampleProf.h:1612:36
    #2 0x564b193ce582 in llvm::sampleprof::ProfileConverter::flattenNestedProfile(llvm::sampleprof::SampleProfileMap&, llvm::sampleprof::FunctionSamples const&) /home/ayermolo/local/upstream-llvm/llvm-project/llvm/include/llvm/ProfileData/SampleProf.h:1607:9
    #3 0x564b193ca9cc in llvm::sampleprof::ProfileConverter::flattenProfile(llvm::sampleprof::SampleProfileMap const&, llvm::sampleprof::SampleProfileMap&, bool) /home/ayermolo/local/upstream-llvm/llvm-project/llvm/include/llvm/ProfileData/SampleProf.h:1558:9
    #4 0x564b1936a0f6 in (anonymous namespace)::SampleProfileMatcher::SampleProfileMatcher(llvm::Module&, llvm::sampleprof::SampleProfileReader&, llvm::PseudoProbeManager const*) /home/ayermolo/local/upstream-llvm/llvm-project/llvm/lib/Transforms/IPO/SampleProfile.cpp:466:7
    #5 0x564b1936a0f6 in std::__1::__unique_if<(anonymous namespace)::SampleProfileMatcher>::__unique_single std::__1::make_unique[abi:v180000]<(anonymous namespace)::SampleProfileMatcher, llvm::Module&, llvm::sampleprof::SampleProfileReader&, llvm::PseudoProbeManager*>(llvm::Module&, llvm::sampleprof::SampleProfileReader&, llvm::PseudoProbeManager*&&) /home/ayermolo/local/upstream-llvm/llvm-project/build_libcxx/include/c++/v1/__memory/unique_ptr.h:685:30
    #6 0x564b1936a0f6 in (anonymous namespace)::SampleProfileLoader::doInitialization(llvm::Module&, llvm::AnalysisManager<llvm::Function>*) /home/ayermolo/local/upstream-llvm/llvm-project/llvm/lib/Transforms/IPO/SampleProfile.cpp:2105:9
    #7 0x564b1936a0f6 in llvm::SampleProfileLoaderPass::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) /home/ayermolo/local/upstream-llvm/llvm-project/llvm/lib/Transforms/IPO/SampleProfile.cpp:2632:21
    #8 0x564b18f338ac in llvm::detail::PassModel<llvm::Module, llvm::SampleProfileLoaderPass, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) /home/ayermolo/local/upstream-llvm/llvm-project/llvm/include/llvm/IR/PassManagerInternal.h:89:17
    #9 0x564b12a1306b in llvm::PassManager<llvm::Module, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) /home/ayermolo/local/upstream-llvm/llvm-project/llvm/include/llvm/IR/PassManager.h:517:40
    #10 0x564b15b5e1a7 in (anonymous namespace)::EmitAssemblyHelper::RunOptimizationPipeline(clang::BackendAction, std::__1::unique_ptr<llvm::raw_pwrite_stream, std::__1::default_delete<llvm::raw_pwrite_stream>>&, std::__1::unique_ptr<llvm::ToolOutputFile, std::__1::default_delete<llvm::ToolOutputFile>>&) /home/ayermolo/local/upstream-llvm/llvm-project/clang/lib/CodeGen/BackendUtil.cpp:1101:9
    #11 0x564b15b447fd in (anonymous namespace)::EmitAssemblyHelper::EmitAssembly(clang::BackendAction, std::__1::unique_ptr<llvm::raw_pwrite_stream, std::__1::default_delete<llvm::raw_pwrite_stream>>) /home/ayermolo/local/upstream-llvm/llvm-project/clang/lib/CodeGen/BackendUtil.cpp:1158:3
    #12 0x564b15b447fd in clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::HeaderSearchOptions const&, clang::CodeGenOptions const&, clang::TargetOptions const&, clang::LangOptions const&, llvm::StringRef, llvm::Module*, clang::BackendAction, llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>, std::__1::unique_ptr<llvm::raw_pwrite_stream, std::__1::default_delete<llvm::raw_pwrite_stream>>) /home/ayermolo/local/upstream-llvm/llvm-project/clang/lib/CodeGen/BackendUtil.cpp:1321:13
    #13 0x564b16f72ca1 in clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) /home/ayermolo/local/upstream-llvm/llvm-project/clang/lib/CodeGen/CodeGenAction.cpp:386:7
    #14 0x564b1d01a29b in clang::ParseAST(clang::Sema&, bool, bool) /home/ayermolo/local/upstream-llvm/llvm-project/clang/lib/Parse/ParseAST.cpp:176:13
    #15 0x564b16be4f7e in clang::FrontendAction::Execute() /home/ayermolo/local/upstream-llvm/llvm-project/clang/lib/Frontend/FrontendAction.cpp:1063:8
    #16 0x564b1695cd8d in clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) /home/ayermolo/local/upstream-llvm/llvm-project/clang/lib/Frontend/CompilerInstance.cpp:1054:33
    #17 0x564b16f50383 in clang::ExecuteCompilerInvocation(clang::CompilerInstance*) /home/ayermolo/local/upstream-llvm/llvm-project/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:272:25
    #18 0x564b0f3bb128 in cc1_main(llvm::ArrayRef<char const*>, char const*, void*) /home/ayermolo/local/upstream-llvm/llvm-project/clang/tools/driver/cc1_main.cpp:249:15
    #19 0x564b0f3b0d6a in ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&, llvm::ToolContext const&) /home/ayermolo/local/upstream-llvm/llvm-project/clang/tools/driver/driver.cpp:366:12
    #20 0x564b0f3abb78 in clang_main(int, char**, llvm::ToolContext const&) /home/ayermolo/local/upstream-llvm/llvm-project/clang/tools/driver/driver.cpp:407:12
    #21 0x564b0f3ec39c in main /home/ayermolo/local/llvm-build-upstream-msan-release/tools/clang/tools/driver/clang-driver.cpp:15:10
    #22 0x7f73e6a3ad84 in __libc_start_main (/lib64/libc.so.6+0x3ad84) (BuildId: 1356e140fb964a20b0d2838960ee69ca6faeb034)
    #23 0x564b0f31642d in _start (/data/users/ayermolo/llvm-build-upstream-msan-release/bin/clang-18+0x315042d)

  Uninitialized value was created by a heap deallocation
    #0 0x564b0f3a2269 in operator delete(void*, std::align_val_t) /home/ayermolo/local/upstream-llvm/llvm-project/compiler-rt/lib/msan/msan_new_delete.cpp:90:3
    #1 0x564b193d94e5 in llvm::DenseMapBase<llvm::DenseMap<llvm::hash_code, llvm::sampleprof::FunctionSamples, llvm::DenseMapInfo<llvm::hash_code, void>, llvm::detail::DenseMapPair<llvm::hash_code, llvm::sampleprof::FunctionSamples>>, llvm::hash_code, llvm::sampleprof::FunctionSamples, llvm::DenseMapInfo<llvm::hash_code, void>, llvm::detail::DenseMapPair<llvm::hash_code, llvm::sampleprof::FunctionSamples>>::grow(unsigned int) /home/ayermolo/local/upstream-llvm/llvm-project/llvm/include/llvm/ADT/DenseMap.h:564:36
    #2 0x564b193d94e5 in llvm::detail::DenseMapPair<llvm::hash_code, llvm::sampleprof::FunctionSamples>* llvm::DenseMapBase<llvm::DenseMap<llvm::hash_code, llvm::sampleprof::FunctionSamples, llvm::DenseMapInfo<llvm::hash_code, void>, llvm::detail::DenseMapPair<llvm::hash_code, llvm::sampleprof::FunctionSamples>>, llvm::hash_code, llvm::sampleprof::FunctionSamples, llvm::DenseMapInfo<llvm::hash_code, void>, llvm::detail::DenseMapPair<llvm::hash_code, llvm::sampleprof::FunctionSamples>>::InsertIntoBucketImpl<llvm::hash_code>(llvm::hash_code const&, llvm::hash_code const&, llvm::detail::DenseMapPair<llvm::hash_code, llvm::sampleprof::FunctionSamples>*) /home/ayermolo/local/upstream-llvm/llvm-project/llvm/include/llvm/ADT/DenseMap.h
    #3 0x564b193d94e5 in llvm::detail::DenseMapPair<llvm::hash_code, llvm::sampleprof::FunctionSamples>* llvm::DenseMapBase<llvm::DenseMap<llvm::hash_code, llvm::sampleprof::FunctionSamples, llvm::DenseMapInfo<llvm::hash_code, void>, llvm::detail::DenseMapPair<llvm::hash_code, llvm::sampleprof::FunctionSamples>>, llvm::hash_code, llvm::sampleprof::FunctionSamples, llvm::DenseMapInfo<llvm::hash_code, void>, llvm::detail::DenseMapPair<llvm::hash_code, llvm::sampleprof::FunctionSamples>>::InsertIntoBucket<llvm::hash_code const&, llvm::sampleprof::FunctionSamples const&>(llvm::detail::DenseMapPair<llvm::hash_code, llvm::sampleprof::FunctionSamples>*, llvm::hash_code const&, llvm::sampleprof::FunctionSamples const&) /home/ayermolo/local/upstream-llvm/llvm-project/llvm/include/llvm/ADT/DenseMap.h:574:17

SUMMARY: MemorySanitizer: use-of-uninitialized-value /home/ayermolo/local/upstream-llvm/llvm-project/llvm/include/llvm/ProfileData/SampleProf.h:959:30 in llvm::sampleprof::FunctionSamples::getHeadSamplesEstimate() const
Exiting

MSAN output. Does this ring any bells?

This diff is causing clang to crash on one of our builds, when clang is linked with jemalloc.
-DCMAKE_EXE_LINKER_FLAGS="-L /usr/lib64 -Wl,-rpath,/usr/lib64 -ljemalloc" \

I am somewhat confused about how this is a performance regression. Doesn't sound like it currently working in multi threaded environment anyway. If it is implemented as is, I don't believe most tools that process debug information are multi threaded, and even if they are majority of time is spent actually parsing debug information.

addressed comments

cleaned up dwarf5-loclist.s

fixed nit

In D157459#4580929, @dblaikie wrote:

In D157459#4578926, @ayermolo wrote:

In D157459#4573865, @dblaikie wrote:

Not sure how I feel about adding multithreading at this level, versus in a wrapper API for those that need it? (insert an interface (DWARFContext, then rename the current concrete implementation as DWARFContextImpl) then have a ThreadSafeDWARFContext that wraps (either as a template, or with a unique_ptr<DWARFContext> member) another DWARFContext and adds the locking) Though it's probably not the end of the world to add it here directly.

Can probably use Class Template Argument Deduction when locking, like this:

std::unique_lock LockGuard(Mutex);

Is there a downside just adding it here, instead of adding complexity of another class and wrapper APIs?

Nothing drastic that I can think of - though perhaps a more vague "DWARFContext is already... not great (exactly which entry points you should use when, what guarantees they provide, when you need to parse things up-front versus lazily, how the memory is managed (mostly it just grows unbounded for instance/never gets cleaned up short of throwing the whole context away) and I worry that trying to figure out when/where multithreaded use is valid or not will add another dimension on an already uncertain and complex interface". But it's probably not the straw that breaks the camel's back, as it were - and adding a wrapper isn't really going to fix the underlying problems/make them especially easier to fix (it's probably a fairly broad rewrite at some point).

Fixed dwarf version in commands

In D157459#4573865, @dblaikie wrote:

Not sure how I feel about adding multithreading at this level, versus in a wrapper API for those that need it? (insert an interface (DWARFContext, then rename the current concrete implementation as DWARFContextImpl) then have a ThreadSafeDWARFContext that wraps (either as a template, or with a unique_ptr<DWARFContext> member) another DWARFContext and adds the locking) Though it's probably not the end of the world to add it here directly.

Can probably use Class Template Argument Deduction when locking, like this:

std::unique_lock LockGuard(Mutex);

Does this need a test?

addressed comments

addressed comments

Depends on
https://reviews.llvm.org/D157355
https://reviews.llvm.org/D157356

Restored formating in rest of the file

rebase

addressed comments

added how gc assembly files were generated

Added test, addressed comments.

updated warning check

addressed some of the comments

I don't remember anything about this code. :)
Can you add a test, and I guess this was submitted through web UI, can you do it with -999 so surrounding code can be viewed?