This is an archive of the discontinued LLVM Phabricator instance.

[clang] Add serialization support for the DynamicAllocLValue variant of APValue::LValueBase::Ptr

Authored by nridge on Jul 4 2023, 9:57 PM.

Diff Detail

Event Timeline

nridge created this revision.Jul 4 2023, 9:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 4 2023, 9:57 PM
nridge updated this revision to Diff 537970.Jul 6 2023, 8:42 PM

Add missing check for isDynamicAlloc

nridge edited the summary of this revision. (Show Details)Jul 8 2023, 3:16 PM
nridge published this revision for review.Jul 8 2023, 3:17 PM

Requesting review.

The patch does not include an automated test because this is fixing a bug report with no test case (I spotted the issue by inspection based on the stack trace). The bug reporter did try the patch locally and confirm that it fixes the crash on their codebase.

Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2023, 3:17 PM

I believe the fix is correct.

Though the fix has been verified by a real-world example, I think it would be nice to get a reproducible testcase. Looking at the stacktrace:

  • the crash occurs during the pch deserialization
  • and we miss handling the case where LValue base is a DynamicAllocLValue

so it looks like we can construct a DynamicAllocLValue case, and try to deserialize it (e.g. emitting a pch), and it should reproduce the crash.

I gave it a try, here is a testcase that triggers the crash

// clang -cc1 -emit-pch -o /tmp/t.pch /tmp/t.cpp
#ifndef HEADER
#define HEADER

struct A {  int *p; };
const A &w = A{ new int(10) };

nridge planned changes to this revision.Jul 11 2023, 11:27 PM

Thank you for the suggested testcase!

nridge updated this revision to Diff 542793.Jul 21 2023, 1:07 AM

Add testcase

hokein accepted this revision.Jul 21 2023, 1:27 AM

Thanks, looks good to me.

This revision is now accepted and ready to land.Jul 21 2023, 1:27 AM
This revision was landed with ongoing or failed builds.Jul 21 2023, 8:17 PM
This revision was automatically updated to reflect the committed changes.