Fznamznon wrote:
Soo, I restored all the attributes on vector deleting destructor and the
calling convention (so they are the same as in non-failing case) and the crash
is gone. I'll put a PR reapplying the patch soon. The reduction helped a lot,
thanks!
https://github.com/llvm/llvm-project/p
Fznamznon wrote:
Removing `static ` from `newUnicodeStringArray` also makes the crash go away...
https://github.com/llvm/llvm-project/pull/126240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cf
Fznamznon wrote:
Thank you for your help @rnk @zmodem !
https://github.com/llvm/llvm-project/pull/126240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
zmodem wrote:
A few notes from looking today.
Reduced main a bit further:
```
int main(int argc, const char** argv) {
volatile auto p = &FilteredBreakIteratorBuilder::createEmptyInstance;
icu::UnicodeString* St = new icu::UnicodeString("abacabadabacab", 15);
delete St;
printf("OKAY\n");
rnk wrote:
I'll try to take a look at this when I get a chance, but that looks like it's
not happening today.
I read the comments a bit and found some codesearch links worth sharing for
reference. Here's the original UnicodeString array new operation:
https://source.chromium.org/chromium/chrom
Fznamznon wrote:
I reduced main code to
```
#include "unicode/filteredbrk.h"
#include
using namespace icu;
int main(int argc, const char** argv) {
UErrorCode status = U_ZERO_ERROR;
//BreakIterator* bi = BreakIterator::createWordInstance(root, status); // Any
iterator will do, make sure fi
Fznamznon wrote:
The problem also is only reproducible when using -start-lib -end-lib options
around icu library object files in the linking command.
https://github.com/llvm/llvm-project/pull/126240
___
cfe-commits mailing list
cfe-commits@lists.llvm.
Fznamznon wrote:
I also noticed that removing -DU_STATIC_IMPLEMENTATION macro passing fixes the
crash.
https://github.com/llvm/llvm-project/pull/126240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/list
Fznamznon wrote:
So, the body of the destructor is not the problem. I commented out all new
additions to body emission of the destructor and it still crashes.
What makes the difference is the alias emission. I commented out alias emission
and it started passing with "_E" destructor always emitt
Fznamznon wrote:
> I've uploaded a stand-alone from-source reproducer of the run time crash
> here: https://crbug.com/402425841#comment10
Reproduced. Seems limited to 32-bit build (only reproducible with -m32 option)
and lld linker. Fine with either standard linker or 64-bit build.
https://gi
Fznamznon wrote:
> I think the `.SCOV$M` section globals should be in the
> `??_EClassA@@UEAAPEXI@Z` comdat group. I think sanitizer coverage (sancov) is
> one of the lesser-used sanitizers here, and it may lack some sophistication
> when it comes to comdat groups. I wasn't aware of this creat
zmodem wrote:
> but, that is still not a great reproducer for a reverted patch. Especially if
> the link issue is due to a missing support in linker. I would appreciate if
> you could maybe point to how exactly UnicodeString is used in failing
> scenario.
I'm sorry that we didn't catch this p
rnk wrote:
I think the `.SCOV$M` section globals should be in the `??_EClassA@@UEAAPEXI@Z`
comdat group. I think sanitizer coverage (sancov) is one of the lesser-used
sanitizers here, and it may lack some sophistication when it comes to comdat
groups. I wasn't aware of this creative use of `bl
Fznamznon wrote:
Reduced linkage failure reproducer (can be reduced further, I'm sure), it
consists of 4 files
t.h:
```
#include
struct Base {
Base() {}
virtual ~Base() {}
};
struct ClassA : public Base {
inline ClassA();
virtual ~ClassA(){} ;
};
inline
ClassA::ClassA() {}
vo
zmodem wrote:
> It turns out another issue (https://crbug.com/402425841) also bisected to
> this PR. That one is a run-time problem, so it may be trickier to figure out,
> but I will look into it next.
The bugs seem related by both involving the ICU library, and if squinting a bit
it seems th
Fznamznon wrote:
> I've put lld-link repro tarballs before and after the llvm change here:
> https://issues.chromium.org/u/1/issues/402354446#comment8
Thanks, I'll take a look!
https://github.com/llvm/llvm-project/pull/126240
___
cfe-commits mailing
zmodem wrote:
I've put lld-link repro tarballs before and after the llvm change here:
https://issues.chromium.org/u/1/issues/402354446#comment8
https://github.com/llvm/llvm-project/pull/126240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ht
AaronBallman wrote:
> Yes, I'm prioritizing this today. I had hoped filteredbrk.obj was enough, but
> I'm working on uploading full linker repros.
>
> It turns out another issue (https://crbug.com/402425841) also bisected to
> this PR. That one is a run-time problem, so it may be trickier to f
zmodem wrote:
Yes, I'm prioritizing this today. I had hoped filteredbrk.obj was enough, but
I'm working on uploading full linker repros.
It turns out another issue (https://crbug.com/402425841) also bisected to this
PR. That one is a run-time problem, so it may be trickier to figure out, but I
AaronBallman wrote:
> We're hitting link errors after this change when building with sanitizer
> coverage enabled:
>
> ```
> lld-link: error: relocation against symbol in discarded section: .text
> >>> referenced by obj/third_party/icu/icuuc_private/filteredbrk.obj:(.SCOVP$M)
> >>> referenced b
Fznamznon wrote:
@zmodem , are you able to reproduce the link issue with just the
`filteredbrk.obj` ? I can't spot a problem in LLVM IR or ASM emitted from it
and lld just gives a bunch of "undefined symbol" link errors. Is that possible
that more files are needed to reproduce?
https://githu
Fznamznon wrote:
> Can you take a look?
Thanks for reporting this. I'll take a look.
https://github.com/llvm/llvm-project/pull/126240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
zmodem wrote:
We're hitting link errors after this change when building with sanitizer
coverage enabled:
```
lld-link: error: relocation against symbol in discarded section: .text
>>> referenced by obj/third_party/icu/icuuc_private/filteredbrk.obj:(.SCOVP$M)
>>> referenced by obj/third_party/ic
Fznamznon wrote:
> Thanks! I approved it.
Thanks! I'll wait with a merge till next week in case someone else has comments.
https://github.com/llvm/llvm-project/pull/126240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/
https://github.com/rnk approved this pull request.
Thanks! I approved it.
https://github.com/llvm/llvm-project/pull/126240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/rnk commented:
I resolved two conversations, but there are two actionable comments.
https://github.com/llvm/llvm-project/pull/126240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listi
https://github.com/rnk commented:
Hm, these comments didn't post. They may be stale. I will post them and revisit
them.
https://github.com/llvm/llvm-project/pull/126240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi
@@ -7919,3 +7919,38 @@ void CodeGenModule::moveLazyEmissionStates(CodeGenModule
*NewBuilder) {
NewBuilder->ABI->MangleCtx = std::move(ABI->MangleCtx);
}
+
+bool CodeGenModule::classNeedsVectorDestructor(const CXXRecordDecl *RD) {
+ CXXDestructorDecl *Dtor = RD->getDestruct
@@ -7919,3 +7919,38 @@ void CodeGenModule::moveLazyEmissionStates(CodeGenModule
*NewBuilder) {
NewBuilder->ABI->MangleCtx = std::move(ABI->MangleCtx);
}
+
+bool CodeGenModule::classNeedsVectorDestructor(const CXXRecordDecl *RD) {
+ CXXDestructorDecl *Dtor = RD->getDestruct
@@ -575,6 +576,12 @@ class CGCXXABI {
QualType ElementType, llvm::Value *&NumElements,
llvm::Value *&AllocPtr, CharUnits &CookieSize);
+ /// Reads the array cookie associated with the given pointer,
+ /// that sho
@@ -2657,7 +2657,10 @@ class VFTableBuilder {
MethodVFTableLocation Loc(MI.VBTableIndex, WhichVFPtr.getVBaseWithVPtr(),
WhichVFPtr.NonVirtualOffset, MI.VFTableIndex);
if (const CXXDestructorDecl *DD = dyn_cast(MD)) {
-MethodVF
https://github.com/rnk edited https://github.com/llvm/llvm-project/pull/126240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
zmodem wrote:
> this is a high-risk change to keep in mind if any Windows-specific issues
> arise.
Thanks for the heads up!
https://github.com/llvm/llvm-project/pull/126240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.or
@@ -7919,3 +7919,38 @@ void CodeGenModule::moveLazyEmissionStates(CodeGenModule
*NewBuilder) {
NewBuilder->ABI->MangleCtx = std::move(ABI->MangleCtx);
}
+
+bool CodeGenModule::classNeedsVectorDestructor(const CXXRecordDecl *RD) {
+ CXXDestructorDecl *Dtor = RD->getDestruct
https://github.com/Fznamznon edited
https://github.com/llvm/llvm-project/pull/126240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -7919,3 +7919,38 @@ void CodeGenModule::moveLazyEmissionStates(CodeGenModule
*NewBuilder) {
NewBuilder->ABI->MangleCtx = std::move(ABI->MangleCtx);
}
+
+bool CodeGenModule::classNeedsVectorDestructor(const CXXRecordDecl *RD) {
+ CXXDestructorDecl *Dtor = RD->getDestruct
rnk wrote:
Thanks for the updates, I think this is almost done.
@zmodem , this will probably have some impact on Chrome code size and will
probably churn many crash stack traces. I think it's safe to rely on all the
existing automated testing, but this is a high-risk change to keep in mind if
@@ -1407,6 +1413,8 @@ llvm::GlobalValue::LinkageTypes
MicrosoftCXXABI::getCXXDestructorLinkage(
// and are emitted everywhere they are used. They are internal if the class
// is internal.
return llvm::GlobalValue::LinkOnceODRLinkage;
+ case Dtor_VectorDeleting:
+
@@ -448,7 +453,8 @@ class ItaniumCXXABI : public CodeGen::CGCXXABI {
if (!IsInlined)
continue;
- StringRef Name = CGM.getMangledName(VtableComponent.getGlobalDecl());
+ StringRef Name =
+ CGM.getMangledName(VtableComponent.getGlobalDecl(fa
@@ -1592,8 +1666,12 @@ namespace {
bool ReturnAfterDelete) {
llvm::BasicBlock *callDeleteBB = CGF.createBasicBlock("dtor.call_delete");
llvm::BasicBlock *continueBB = CGF.createBasicBlock("dtor.continue");
-llvm::Value *ShouldCal
@@ -7919,3 +7919,38 @@ void CodeGenModule::moveLazyEmissionStates(CodeGenModule
*NewBuilder) {
NewBuilder->ABI->MangleCtx = std::move(ABI->MangleCtx);
}
+
+bool CodeGenModule::classNeedsVectorDestructor(const CXXRecordDecl *RD) {
+ CXXDestructorDecl *Dtor = RD->getDestruct
https://github.com/Fznamznon updated
https://github.com/llvm/llvm-project/pull/126240
>From 04636bea1b873805af02dea865637d7125cee1e5 Mon Sep 17 00:00:00 2001
From: "Podchishchaeva, Mariya"
Date: Thu, 12 Dec 2024 08:57:37 -0800
Subject: [PATCH 1/3] [MS][clang] Add support for vector deleting des
@@ -2013,7 +2023,7 @@ llvm::Value *MicrosoftCXXABI::EmitVirtualDestructorCall(
ASTContext &Context = getContext();
llvm::Value *ImplicitParam = llvm::ConstantInt::get(
llvm::IntegerType::getInt32Ty(CGF.getLLVMContext()),
- DtorType == Dtor_Deleting);
+ 2 * (
@@ -1433,6 +1433,83 @@ static bool
CanSkipVTablePointerInitialization(CodeGenFunction &CGF,
return true;
}
+namespace {
+llvm::Value *LoadThisForDtorDelete(CodeGenFunction &CGF,
Fznamznon wrote:
I moved it back. It seems in some earlier iteration of the pa
@@ -4053,6 +4063,19 @@ void MicrosoftCXXABI::emitCXXStructor(GlobalDecl GD) {
if (GD.getDtorType() == Dtor_Base && !CGM.TryEmitBaseDestructorAsAlias(dtor))
return;
+ if (GD.getDtorType() == Dtor_VectorDeleting &&
+ !CGM.classNeedsVectorDestructor(dtor->getParent())
@@ -1433,6 +1433,83 @@ static bool
CanSkipVTablePointerInitialization(CodeGenFunction &CGF,
return true;
}
+namespace {
+llvm::Value *LoadThisForDtorDelete(CodeGenFunction &CGF,
+ const CXXDestructorDecl *DD) {
+ if (Expr *ThisArg = DD->ge
@@ -1366,9 +1372,9 @@ bool ItaniumCXXABI::isZeroInitializable(const
MemberPointerType *MPT) {
/// at entry -2 in the vtable.
void ItaniumCXXABI::emitVirtualObjectDelete(CodeGenFunction &CGF,
const CXXDeleteExpr *DE,
-
Fznamznon wrote:
Ping.
https://github.com/llvm/llvm-project/pull/126240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/Fznamznon updated
https://github.com/llvm/llvm-project/pull/126240
>From 04636bea1b873805af02dea865637d7125cee1e5 Mon Sep 17 00:00:00 2001
From: "Podchishchaeva, Mariya"
Date: Thu, 12 Dec 2024 08:57:37 -0800
Subject: [PATCH 1/2] [MS][clang] Add support for vector deleting des
github-actions[bot] wrote:
:warning: C/C++ code formatter, clang-format found issues in your code.
:warning:
You can test this locally with the following command:
``bash
git-clang-format --diff 83ba3740bf51347307494d013099e392c310e32b
04636bea1b873805af02dea865637d7125cee1e5 --e
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: Mariya Podchishchaeva (Fznamznon)
Changes
Whereas it is UB in terms of the standard to delete an array of objects via
pointer whose static type doesn't match its dynamic type, MSVC supports an
extension allowing to do it.
Aside from array
llvmbot wrote:
@llvm/pr-subscribers-debuginfo
@llvm/pr-subscribers-clang-codegen
Author: Mariya Podchishchaeva (Fznamznon)
Changes
Whereas it is UB in terms of the standard to delete an array of objects via
pointer whose static type doesn't match its dynamic type, MSVC supports an
extens
https://github.com/Fznamznon created
https://github.com/llvm/llvm-project/pull/126240
Whereas it is UB in terms of the standard to delete an array of objects via
pointer whose static type doesn't match its dynamic type, MSVC supports an
extension allowing to do it.
Aside from array deletion no
53 matches
Mail list logo
