This is an archive of the discontinued LLVM Phabricator instance.

MC/DC in LLVM Source-Based Code Coverage: LLVM back-end and compiler-rt
ClosedPublic

Authored by alanphipps on Nov 28 2022, 11:54 AM.

Details

Summary

MC/DC in LLVM Source-based Code Coverage: Review 1/3

Background
I previously upstreamed work to enable Branch Coverage (https://reviews.llvm.org/D84467), which was pushed in early 2021. MC/DC (Modified Condition/Decision Coverage) is a planned enhancement to Source-based Code Coverage. Implementation was completed in May for our downstream Arm compiler, and in general use it has not yielded any issues.

Patches have been tested on windows (stage1) and linux/mac (stage1 and stage2). Fuchsia is not yet supported.

See attached file for Background, Motivation, Design, and Implementation Concepts:

See attached PDF for Technical Talk slides from 2022 LLVM Developers' Meeting:

I am also available at Discord @alanphipps

Review
This review is for the LLVM back-end processing and profile reading/writing components. compiler-rt changes are included.

  • Instrumentation intrinsic lowering and section allocation:
    • IntrinsicInst.h
    • Intrinsics.td
    • IntrinsicInst.cpp
    • InstrProfiling.h, InstrProfiling.cpp
  • Profiling format read/write:
    • CoverageMappingWriter.cpp
    • CoverageMappingReader.cpp
  • Profile read/write:
    • InstrProf.h, InstrProf.cpp
    • InstrProfWriter.h, InstrProfWriter.cpp
    • InstrProfReader.h, InstrProfReader.cpp
  • Runtime profiling aids:
    • InstrProfiling.h
    • InstrProfilingFilebaremetal.c
    • InstrProfilingPlatformBaremetal.c
    • compiler-rt

Diff Detail

Event Timeline

alanphipps created this revision.Nov 28 2022, 11:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2022, 11:54 AM
alanphipps requested review of this revision.Nov 28 2022, 11:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2022, 11:54 AM
alanphipps edited the summary of this revision. (Show Details)Nov 28 2022, 12:11 PM
alanphipps edited the summary of this revision. (Show Details)Nov 28 2022, 12:16 PM

Please correct me if I'm wrong, but my understanding is that we have a single bitvector per expression (or function?) to track coverage. And that these bitvectors are at most 64 bits long. Is the number of these bitvectors per function known at compile time? I think yes because you are allocating the IPSK_bitmap section at compile time.

If this is the case then I would suggest extending the IPSK_cnts rather than creating a new IPSK_bitmap section. There is lots of logic to track the location of the IPSK_cnts and correlate its values to the correct functions (this is the main feature of Debug Info Correlation). Instead, we could allocate more data for function counters and say the last N bytes are for the MC/DC bitvectors. The lowered code would know where to write the data because they are given the address and the raw profile readers would know when to expect this data at the end of the counters because they know N.
This would simplify INSTR_PROF_DATA because we would only need NumBitmapBytes and not RelativeBitmapPtr, we wouldn't need to extend the raw header, we wouldn't need the IPSK_bitmap section, and the llvm.instrprof.mcdc. intrinsics could be more closely aligned to the other llvm.instrprof. intrinsics.

I'm open to other ideas/arguments so let me know what you think.

compiler-rt/include/profile/InstrProfData.inc
658

Do we also need to bump the version for the raw files since we added to its header?

llvm/include/llvm/IR/IntrinsicInst.h
1437

I don't think this field applies to other instrprof intrinsics. Can we create a new base class exclusively for MCDC intrinsics and add this method there instead?

1509

You'll also want to add these new intrinsics to llvm/docs/LangRef.rst .

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7200

Did you mean to add ::instrprof_mcdc_parameters here too?

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
816

I'm not seeing any tests in llvm/test/Instrumentation/InstrProfiling/ to test that this code was lowered correctly. Can you add some?

It would also be good to add tests in clang/test/Profile or llvm/test/Transforms/PGOProfile/ to make sure these intrinsics are added to the correct locations (I actually don't see where InstrProfMCDCTVBitmapUpdate is created).

828

I've been told that register names could increase memory usage and it's best not to add them unless they are very useful for debugging.

854–864

There is an option to make the increment code atomic. See lowerIncrement(). Are you interested in supporting this option? Though I'm not sure if there is an atomic operation that could do this.

Hi @ellis, thank you for looking at this deeper! I'll update based on your comments below, but I want to respond to your primary question first:

Please correct me if I'm wrong, but my understanding is that we have a single bitvector per expression (or function?) to track coverage. And that these bitvectors are at most 64 bits long. Is the number of these bitvectors per function known at compile time? I think yes because you are allocating the IPSK_bitmap section at compile time.

Yes, a bitvector (or bitmap) is allocated per expression for each function, and this is known at compile time. Each one can be at minimum 8 bits and 64bits max, although the max value could be extended in the future.

If this is the case then I would suggest extending the IPSK_cnts rather than creating a new IPSK_bitmap section. There is lots of logic to track the location of the IPSK_cnts and correlate its values to the correct functions (this is the main feature of Debug Info Correlation). Instead, we could allocate more data for function counters and say the last N bytes are for the MC/DC bitvectors. The lowered code would know where to write the data because they are given the address and the raw profile readers would know when to expect this data at the end of the counters because they know N.

I totally understand the desire to avoid modifying the raw header and the additional complexity, and so I'm glad you honed in on it so we could explore how best to simplify this.

I did consider extending IPSK_cnts, and what you describe ought to work functionally just fine, but the reason I opted not to do that was because of considerations for efficient memory utilization at link time (I expect MC/DC will be getting the most utilization for embedded, where this is most important). With the alignment of IPSK_cnts being driven by the size of the counters (up to 64bits, as you know, and for good reason), even 1 byte of additional MC/DC data at the end of a function's IPSK_cnts would yield large gaps during linker aggregation of these sections across functions due to alignment constraints. With one of the goals behind MC/DC being to avoid unnecessary memory consumption, it didn't make sense philosophically to have anything related to memory efficient bitmap bits be impacted by the alignment and size of the counters, which, at least conceptually, is an altogether different thing. Keeping the bitmaps in a separate, 1-byte aligned section IPSK_bits yields the most granularity and flexibility. I tried to abstract a lot of the overlap for how the sections are created as much as possible.

With respect to modifying the raw header, I agree there is reluctance to changing this, and if there is a way to derive the needed information without having to do this, I would definitely consider it.

alanphipps added inline comments.Nov 29 2022, 2:24 PM
llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
816

Hmm. Right, I do test the lowering using tests I added in clang/test/Profile (It's in the clang-specific review, but obviously it has overlap with what is done in this portion), but I overlooked adding anything specific to llvm/test/Instrumentation/InstrProfiling.

I'll look into adding some additional tests for this, as well as for the intrinsics.

ellis added a comment.Nov 29 2022, 3:13 PM

Hi @ellis, thank you for looking at this deeper! I'll update based on your comments below, but I want to respond to your primary question first:

Please correct me if I'm wrong, but my understanding is that we have a single bitvector per expression (or function?) to track coverage. And that these bitvectors are at most 64 bits long. Is the number of these bitvectors per function known at compile time? I think yes because you are allocating the IPSK_bitmap section at compile time.

Yes, a bitvector (or bitmap) is allocated per expression for each function, and this is known at compile time. Each one can be at minimum 8 bits and 64bits max, although the max value could be extended in the future.

If this is the case then I would suggest extending the IPSK_cnts rather than creating a new IPSK_bitmap section. There is lots of logic to track the location of the IPSK_cnts and correlate its values to the correct functions (this is the main feature of Debug Info Correlation). Instead, we could allocate more data for function counters and say the last N bytes are for the MC/DC bitvectors. The lowered code would know where to write the data because they are given the address and the raw profile readers would know when to expect this data at the end of the counters because they know N.

I totally understand the desire to avoid modifying the raw header and the additional complexity, and so I'm glad you honed in on it so we could explore how best to simplify this.

I did consider extending IPSK_cnts, and what you describe ought to work functionally just fine, but the reason I opted not to do that was because of considerations for efficient memory utilization at link time (I expect MC/DC will be getting the most utilization for embedded, where this is most important). With the alignment of IPSK_cnts being driven by the size of the counters (up to 64bits, as you know, and for good reason), even 1 byte of additional MC/DC data at the end of a function's IPSK_cnts would yield large gaps during linker aggregation of these sections across functions due to alignment constraints. With one of the goals behind MC/DC being to avoid unnecessary memory consumption, it didn't make sense philosophically to have anything related to memory efficient bitmap bits be impacted by the alignment and size of the counters, which, at least conceptually, is an altogether different thing. Keeping the bitmaps in a separate, 1-byte aligned section IPSK_bits yields the most granularity and flexibility. I tried to abstract a lot of the overlap for how the sections are created as much as possible.

With respect to modifying the raw header, I agree there is reluctance to changing this, and if there is a way to derive the needed information without having to do this, I would definitely consider it.

Yes, putting everything in the IPSK_cnts section would lead to more size overhead. In that case, I'm fine with the new IPSK_bitmap section. Thanks for clarifying!

alanphipps marked 7 inline comments as done.

Updated diff based on review comments

alanphipps added inline comments.Dec 23 2022, 11:04 AM
compiler-rt/include/profile/InstrProfData.inc
658

Thank you, I totally overlooked this.

llvm/include/llvm/IR/IntrinsicInst.h
1437

I refactored this into a base class for MCDC and one for Counters.

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
816

I added to llvm/test/Instrumentation/InstrProfiling and also an intrinsics case to clang/test/Profile (in the clang-specific MC/DC review).

828

Thanks, noted. -- I did add the name here because it was definitely useful in debugging. I'm going to keep the name for now unless you or others feel strongly.

854–864

I looked into this, and I think it is possible to enable a atomic RMW version using Or, similar to what is done for Add. It isn't something that I'm particularly interested in at this time, but I can definitely do it as a future patch.

ellis added a comment.Jan 30 2023, 2:16 PM

Just did another quick pass of this diff and I'll take a look at the other diffs soon.

compiler-rt/lib/profile/InstrProfiling.h
105–111
llvm/include/llvm/IR/IntrinsicInst.h
1509

I believe dyn_cast<InstrProfMCDCInstBase>(I) will return nullptr when I is InstrProfMCDCParameters, InstrProfMCDCTVBitmapUpdate, or InstrProfMCDCCondBitmapUpdate as it is right now. To fix this we need to implement classof() for InstrProfMCDCInstBase like in D141579. Just to avoid confusion in the future.

llvm/include/llvm/IR/Intrinsics.td
913–915

NIT: Inconsistent formatting.

llvm/lib/ProfileData/InstrProfReader.cpp
442

Why not set Radix=0 so it can automatically infer the base? Same with the other getAsInteger() below. Then the user can specify these values in the most convenient form.

Also, I'm not sure if getAsInteger() will fail if there are spaces. If it does, should we call .trim() just before so that we can accept more readable inputs? For example, the tests could look something like this:

# Num Bitmask Bytes:
$ 2
# Bitmask Byte Values:
0x1d
0x0
llvm/lib/ProfileData/InstrProfWriter.cpp
718
llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
1086–1087

There seems to be extra whitespace changes here.

llvm/test/tools/llvm-profdata/mcdc-bitmap.test
2

I just recently discovered split-file (https://llvm.org/docs/TestingGuide.html#extra-files). Since we only use the .proftext files in this test we can put their contents here so that everything is in one place. This will make this file much larger, so it's up to you if you think it's worth it.

alanphipps marked 6 inline comments as done.Jun 14 2023, 7:39 AM
alanphipps added inline comments.
llvm/lib/ProfileData/InstrProfReader.cpp
442

Hmm I didn't even think of these. Thanks for the suggestions!

alanphipps marked an inline comment as done.

Rebase and update based on comments.

Reapply clang-format

I added a few inline comments, but it looks like this patch has been in review for a while.

@ellis do you have any issues with this patch going forward?

compiler-rt/lib/profile/InstrProfilingFile.c
259

Would it be appropriate to issue a warning to a user trying to use both MCDC + bias mode?

compiler-rt/lib/profile/InstrProfilingMerge.c
209

Could you explain what exactly you are doing as you iterate over the number of bitmap bytes?

llvm/include/llvm/IR/IntrinsicInst.h
1519

Doxygen?

1546

Doxygen?

1551

This could use a Doxygen comment too?

paquette added inline comments.Jul 2 2023, 11:03 PM
llvm/include/llvm/Transforms/Instrumentation/InstrProfiling.h
142

Can you elaborate on this comment a bit more?

  1. Is this returning a GlobalVariable representing the region bitmaps?
  2. What are the parameters?
llvm/lib/ProfileData/InstrProfReader.cpp
453

you're specifically trying to get an integer here, right?

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
745

Is this TODO still relevant?

891

I like these comments. :)

1055

How about reduce indentation here by using an early return?

if (!UseComdat)
  return;
StringRef GroupName = ...;
1087

Don't need to add braces here

1167–1170

I'm a little surprised this doesn't call createRegionBitmaps. Why is that? Should this have a different name?

1201

Similar to above, I'm surprised there's no code sharing with createRegionCounters here.

1246

Comment?

ellis added a comment.Jul 5 2023, 11:38 AM

All my comments (except my last one) seems to be resolved so looks good to me! After @paquette's latest comments are resolved I'll do a final pass

llvm/test/tools/llvm-profdata/mcdc-bitmap.test
2

I've used split-file a few times since I made this comment and I think it should be used for this test, but let me know what you think

alanphipps added inline comments.Jul 5 2023, 3:23 PM
llvm/test/tools/llvm-profdata/mcdc-bitmap.test
2

Apologies, not sure how, but I overlooked this comment! I agree using split-file would help this test. I wasn't aware of it -- I need to review these guides. I'll make the change.

alanphipps marked 16 inline comments as done.Jul 14 2023, 11:24 AM
alanphipps added inline comments.
compiler-rt/lib/profile/InstrProfilingFile.c
259

Yes, this is related to runtime counter relocation. I will issue a warning from the InstrProfiling pass if this is seen (along with a test case).

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
1167–1170

createRegionBitmaps() is called via setupProfileSection() just below. I'll add a brief comment here to (hopefully) make it clearer.

1201

The goal was to abstract what I could through setupProfileSection(), since sections have to be created for both counters and bitmaps, while still keeping the Counter and Bitmap concepts distinct. Frankly, I think this whole pass could eventually benefit from a rewrite owing to its evolution, but I attempted not to make it worse.

alanphipps marked 3 inline comments as done.
alanphipps added a reviewer: paquette.

Diff update...

paquette accepted this revision.Jul 17 2023, 8:27 PM

I think a couple parts of this patch need clang-format before pushing, but otherwise LGTM.

llvm/lib/ProfileData/InstrProfReader.cpp
726

I think that clang-format will remove the braces here.

This revision is now accepted and ready to land.Jul 17 2023, 8:27 PM

Thank you for working. Could you reformat a few lines?

I know some inconsistencies in *.inc but I think we may follow traditional format there.

llvm/include/llvm/ProfileData/InstrProfData.inc
658

I guess you have forgot regenerating more a few input files.

  • Transforms/PGOProfile/memprof.ll
llvm/lib/ProfileData/InstrProfCorrelator.cpp
203–204

This could be reformatted.

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
750

Could you reformat?

1060–1062

This could be reformatted.

snehasish added inline comments.
llvm/include/llvm/ProfileData/InstrProf.h
1031–1033

Delete this comment?

alanphipps marked 6 inline comments as done.

Update diff based clang-format and review comments...

llvm/include/llvm/ProfileData/InstrProfData.inc
658

Ah, ok -- good catch! I did test with ZLIB enabled prior to when this test was added, but in my normal build, ZLIB is disabled, so it was skipped. This appears to be the only case that otherwise fails. I reached out to the code owners and regenerated the proper inputs for this.

(Edit: code owners just landed a change to use proftext rather than profraw for the profile data).

llvm/lib/ProfileData/InstrProfReader.cpp
726

Hmm, it didn't catch it, but I can make the change.

I will be working around travel over the next couple of weeks, but I plan to start landing these patches in September.

Merge conflicts to origin/main;

Merge conflicts to origin/main;

Yes, I had planned to update all of the patches with a rebase prior to landing anything.

Final rebase

phosek accepted this revision.Sep 19 2023, 3:04 PM

LGTM

This revision was landed with ongoing or failed builds.Sep 19 2023, 3:07 PM
This revision was automatically updated to reflect the committed changes.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 19 2023, 3:07 PM
Herald added subscribers: Restricted Project, cfe-commits, abidh. · View Herald Transcript
akhuang added a subscriber: akhuang.EditedSep 20 2023, 3:09 PM

I'm still working on a repro, but after this patch we're seeing "truncated profile data" errors in chromium (crbug.com/1485303)

I'm still working on a repro, but after this patch we're seeing "truncated profile data" errors in chromium (crbug.com/1485303)

Thank you for the heads-up! If there's an issue, I'm eager to ensure it is addressed. Your repro will be helpful.

hans added a subscriber: hans.Sep 21 2023, 3:21 AM

I'm still working on a repro, but after this patch we're seeing "truncated profile data" errors in chromium (crbug.com/1485303)

Thank you for the heads-up! If there's an issue, I'm eager to ensure it is addressed. Your repro will be helpful.

I added steps to download the profile here: https://bugs.chromium.org/p/chromium/issues/detail?id=1485303#c4

I think this should be reverted while being investigated: https://github.com/llvm/llvm-project/commit/53a2923bf67bc164558d489493176630123abf7e

w2yehia added a subscriber: w2yehia.EditedSep 21 2023, 11:26 AM

Please update InstrProfilingPlatformAIX.c as well, specifically add new dummy vars for the new section.
Edit: I can post the patch if you wish.

alanphipps reopened this revision.Sep 30 2023, 11:40 AM
This revision is now accepted and ready to land.Sep 30 2023, 11:40 AM

Rebase and address build bot failures

I added steps to download the profile here: https://bugs.chromium.org/p/chromium/issues/detail?id=1485303#c4
I think this should be reverted while being investigated: https://github.com/llvm/llvm-project/commit/53a2923bf67bc164558d489493176630123abf7e

Thank you for the repro! It was a huge help. There was in fact a bug in InstrProfReader.cpp where the wrong profile format version was being checked before attempting to read MC/DC bitmap bytes. The check was added to ensure backward compatibility with older versions. I fixed that check and added a testcase to ensure v10 of the format can still be handled successfully.

I just noticed this also broke some lit tests on mac: https://bugs.chromium.org/p/chromium/issues/detail?id=1485487#c0
That's also visible on greendragon: https://green.lab.llvm.org/green/view/Clang/job/clang-stage1-RA/35721/testReport/

The Mac failures were due a missing adjustment in clang to ensure profile sections are properly page aligned. When I separated out the patches to make the process easier, that change ended up in https://reviews.llvm.org/D138849 with the other MC/DC clang changes when it should've been included in this patch. I've added it and verified that the Mac tests pass with this patch.

Please update InstrProfilingPlatformAIX.c as well, specifically add new dummy vars for the new section.
Edit: I can post the patch if you wish.

I just added the dummy vars, but you can have a look. You're welcome to post the patch too and it would be great if you could assist in testing for AIX. Thank you!

Hi @hans @w2yehia Please let me know if you have any additional concerns. I'd like to reland this within the next day or so. Thank you!

@alanphipps thanks for handling the AIX case. From inspection it looks sufficient. I wasn't able to apply the patch with arc, so I'll wait for it to land and report/fix any AIX specific issues.

This revision was landed with ongoing or failed builds.Oct 30 2023, 9:16 AM
This revision was automatically updated to reflect the committed changes.
hans added a comment.Nov 10 2023, 2:28 AM

Just a heads up that we're seeing crashes in the Rust compiler which bisects to this change. Investigating in https://bugs.chromium.org/p/chromium/issues/detail?id=1500558

Code built with older versions of LLVM (e.g. rust) and running with the updated runtime crash at startup with this change.

hans added a comment.Nov 10 2023, 4:17 AM

Just a heads up that we're seeing crashes in the Rust compiler which bisects to this change. Investigating in https://bugs.chromium.org/p/chromium/issues/detail?id=1500558

Here's a small reproducer:

$ cat /tmp/x.ll
target triple = "x86_64-unknown-linux-gnu"

declare void @llvm.instrprof.increment(ptr %0, i64 %1, i32 %2, i32 %3)
@prof_name = private constant [9 x i8] c"prof_name"

define internal void @f() {
  call void @llvm.instrprof.increment(ptr @prof_name, i64 123456, i32 32, i32 0)
  ret void
}

define void @foo() {
  call void @f()
  ret void
}

define void @bar() {
  call void @f()
  ret void
}

$ build/bin/opt /tmp/x.ll -passes='cgscc(inline),instrprof' -S
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.      Program arguments: build/bin/opt /tmp/x.ll -passes=cgscc(inline),instrprof -S
 #0 0x000055fcf4193d48 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (build/bin/opt+0x2f77d48)
 #1 0x000055fcf419186e llvm::sys::RunSignalHandlers() (build/bin/opt+0x2f7586e)
 #2 0x000055fcf41943fd SignalHandler(int) Signals.cpp:0:0
 #3 0x00007f0cbe05a510 (/lib/x86_64-linux-gnu/libc.so.6+0x3c510)
 #4 0x000055fcf3b279c9 llvm::GlobalVariable::eraseFromParent() (build/bin/opt+0x290b9c9)
 #5 0x000055fcf47a87c8 llvm::InstrProfiling::emitNameData() (build/bin/opt+0x358c7c8)
 #6 0x000055fcf47a32e3 llvm::InstrProfiling::run(llvm::Module&, std::function<llvm::TargetLibraryInfo const& (llvm::Function&)>) (build/bin/opt+0x35872e3)
 #7 0x000055fcf47a2b99 llvm::InstrProfiling::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (build/bin/opt+0x3586b99)
 #8 0x000055fcf43a31ed llvm::detail::PassModel<llvm::Module, llvm::InstrProfiling, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (build/bin/opt+0x31871ed)
 #9 0x000055fcf3bacf04 llvm::PassManager<llvm::Module, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (build/bin/opt+0x2990f04)
#10 0x000055fcf1b479db llvm::runPassPipeline(llvm::StringRef, llvm::Module&, llvm::TargetMachine*, llvm::TargetLibraryInfoImpl*, llvm::ToolOutputFile*, llvm::ToolOutputFile*, llvm::ToolOutputFile*, llvm::StringRef, llvm::ArrayRef<llvm::PassPlugin>, llvm::opt_tool::OutputKind, llvm::opt_tool::VerifierKind, bool, bool, bool, bool, bool, bool, bool) (build/bin/opt+0x92b9db)
#11 0x000055fcf1b54e95 main (build/bin/opt+0x938e95)
#12 0x00007f0cbe0456ca __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:74:3
#13 0x00007f0cbe045785 call_init ./csu/../csu/libc-start.c:128:20
#14 0x00007f0cbe045785 __libc_start_main ./csu/../csu/libc-start.c:347:5
#15 0x000055fcf1b4156a _start (build/bin/opt+0x92556a)
Segmentation fault

Just a heads up that we're seeing crashes in the Rust compiler which bisects to this change. Investigating in https://bugs.chromium.org/p/chromium/issues/detail?id=1500558

Thank you for the repro! This was a very straightforward fix; effectively the inlining of an instrumented function into multiple functions led to the creation of duplicate data variables, ultimately leading to a segfault in emitNamedata() when eraseFromParent() was called multiple times from the same NamePtr. Prior to my change to abstract the creation of the data variable, there was no explicit check for this, but it was inadvertently avoided by checking that multiple counter sections were not created.

I created a pull request for the fix here: https://github.com/llvm/llvm-project/pull/71998

Code built with older versions of LLVM (e.g. rust) and running with the updated runtime crash at startup with this change.

FWIW, neither followups fixed this issue. The crash happens in __llvm_profile_instrument_target.

hans added a comment.Nov 21 2023, 2:23 AM

We're seeing crashes in initializeValueProfRuntimeRecord that bisects to this commit. I think Zequan is investigating: https://bugs.chromium.org/p/chromium/issues/detail?id=1503919

Code built with older versions of LLVM (e.g. rust) and running with the updated runtime crash at startup with this change.

FWIW, neither followups fixed this issue. The crash happens in __llvm_profile_instrument_target.

Hmm, then it's different from the repro given earlier. Can you provide a test case?

We're seeing crashes in initializeValueProfRuntimeRecord that bisects to this commit. I think Zequan is investigating: https://bugs.chromium.org/p/chromium/issues/detail?id=1503919

It turns out to be that our rust code with old version of rustc without this change, so mixture of raw profile versions are used, causing segment fault.

We're seeing crashes in initializeValueProfRuntimeRecord that bisects to this commit. I think Zequan is investigating: https://bugs.chromium.org/p/chromium/issues/detail?id=1503919

It turns out to be that our rust code with old version of rustc without this change, so mixture of raw profile versions are used, causing segment fault.

Thank you for the update! Is this also the case of the issue @glandium reports above as well? I think both issues manifested as a ValueProf issue.

We're seeing crashes in initializeValueProfRuntimeRecord that bisects to this commit. I think Zequan is investigating: https://bugs.chromium.org/p/chromium/issues/detail?id=1503919

It turns out to be that our rust code with old version of rustc without this change, so mixture of raw profile versions are used, causing segment fault.

Thank you for the update! Is this also the case of the issue @glandium reports above as well? I think both issues manifested as a ValueProf issue.

I don't know, but I'd suggest to check if all sources compiled with profile runtime are using the llvm version that contains this change. Otherwise, mixed raw profiles versions will cause unexpected behaviour.

We're seeing crashes in initializeValueProfRuntimeRecord that bisects to this commit. I think Zequan is investigating: https://bugs.chromium.org/p/chromium/issues/detail?id=1503919

It turns out to be that our rust code with old version of rustc without this change, so mixture of raw profile versions are used, causing segment fault.

Thank you for the update! Is this also the case of the issue @glandium reports above as well? I think both issues manifested as a ValueProf issue.

I don't know, but I'd suggest to check if all sources compiled with profile runtime are using the llvm version that contains this change. Otherwise, mixed raw profiles versions will cause unexpected behaviour.

I just saw @glandium's earlier comment:

Code built with older versions of LLVM (e.g. rust) and running with the updated runtime crash at startup with this change.

This is the exact same issue we encountered. Because there is a profile format change, it's expected to update both clang and rustc to use the same version of llvm in order for it to work.

FWIW, it's the first time for as long as I remember that mixing LLVM versions causes a runtime crash (that looks like a null deref).

hans added a comment.Wed, Nov 22, 7:30 AM

I just saw @glandium's earlier comment:

Code built with older versions of LLVM (e.g. rust) and running with the updated runtime crash at startup with this change.

This is the exact same issue we encountered. Because there is a profile format change, it's expected to update both clang and rustc to use the same version of llvm in order for it to work.

Thanks for figuring this out!

Would it be possible to somehow make profile format mismatches a linker error instead of a hard-to-debug runtime problem? For example could the instrumentation depend on some symbol in the runtime whose name includes a version number?

I think the ASan (and maybe other sanitizer) instrumentations do this.

MaskRay added a comment.EditedWed, Nov 22, 9:52 AM

I just saw @glandium's earlier comment:

Code built with older versions of LLVM (e.g. rust) and running with the updated runtime crash at startup with this change.

This is the exact same issue we encountered. Because there is a profile format change, it's expected to update both clang and rustc to use the same version of llvm in order for it to work.

Thanks for figuring this out!

Would it be possible to somehow make profile format mismatches a linker error instead of a hard-to-debug runtime problem? For example could the instrumentation depend on some symbol in the runtime whose name includes a version number?

I think the ASan (and maybe other sanitizer) instrumentations do this.

Te detect incompatibilities, we can either rename the metadata section or add a version detector field either to the format header or in individual datum.

Renaming seems infeasible since a lot of places need to be modified (difficult to make an atomic update everywhere).

Adding a version detector to individual datum needs to increase the size (seems not favored) or reuse a field to do more work.

There is a check already, but it's not enough: https://github.com/llvm/llvm-project/issues/52683