[clang] [clang] Add size filter for stack auto init (PR #74777)
https://github.com/vitalybuka closed https://github.com/llvm/llvm-project/pull/74777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Add size filter for stack auto init (PR #74777)
vitalybuka wrote: I guess buildkite/github-pull-requests is stuck https://github.com/llvm/llvm-project/pull/74777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Add size filter for stack auto init (PR #74777)
vitalybuka wrote: @jfbastien @fhahn @efriedma-quic I'd like to merge if no other feedback https://github.com/llvm/llvm-project/pull/74777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Add size filter for stack auto init (PR #74777)
https://github.com/vitalybuka approved this pull request. LGTM, but please wait others feedback https://github.com/llvm/llvm-project/pull/74777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Add size filter for stack auto init (PR #74777)
vitalybuka wrote:
As we discussed offline, it would nice to handle cases like this
```
struct Foo {
int x; // we should try to make sure X is initialized.
char buff[1024]; // this one is fine to skip
};
void main() {
Foo foo;
}
```
But seems moving size check deeper does not help. If so, simpler patch is
preferred.
https://github.com/llvm/llvm-project/pull/74777
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Add size filter for stack auto init (PR #74777)
@@ -0,0 +1,60 @@ +// Pattern related max size tests: 1, 1024, 4096 +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -ftrivial-auto-var-init=pattern -ftrivial-auto-var-init-max-size=1 %s -emit-llvm -o - | FileCheck -check-prefix=PATTERN-COMMON -check-prefix=PATTERN-MAX-1 %s +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -ftrivial-auto-var-init=pattern -ftrivial-auto-var-init-max-size=1024 %s -emit-llvm -o - | FileCheck -check-prefix=PATTERN-COMMON -check-prefix=PATTERN-MAX-1024 %s +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -ftrivial-auto-var-init=pattern -ftrivial-auto-var-init-max-size=4096 %s -emit-llvm -o - | FileCheck -check-prefix=PATTERN-COMMON -check-prefix=PATTERN-MAX-4096 %s +// +// Zero related max size tests: 1, 1024, 4096 +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -ftrivial-auto-var-init=zero -ftrivial-auto-var-init-max-size=1 %s -emit-llvm -o - | FileCheck -check-prefix=ZERO-COMMON -check-prefix=ZERO-MAX-1 %s vitalybuka wrote: it would be nice to test something like `test_huge_larger_init`, to show that user requested initialization is preserved. https://github.com/llvm/llvm-project/pull/74777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Add size filter for stack auto init (PR #74777)
https://github.com/vitalybuka edited https://github.com/llvm/llvm-project/pull/74777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Add size filter for stack auto init (PR #74777)
@@ -1205,10 +1205,19 @@ static void emitStoresForConstant(CodeGenModule , const VarDecl , } auto *SizeVal = llvm::ConstantInt::get(CGM.IntPtrTy, ConstantSize); + auto trivialAutoVarInitMaxSize = vitalybuka wrote: would it be simpler ``` if (IsAutoInit && sizeVal->getValue().sgt(trivialAutoVarInitMaxSize)) return; ``` https://github.com/llvm/llvm-project/pull/74777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Add size filter for stack auto init (PR #74777)
https://github.com/haopliu edited https://github.com/llvm/llvm-project/pull/74777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Add size filter for stack auto init (PR #74777)
@@ -1759,20 +1759,29 @@ void
CodeGenFunction::emitZeroOrPatternForAutoVarInit(QualType type,
const VarDecl ,
Address Loc) {
auto trivialAutoVarInit = getContext().getLangOpts().getTrivialAutoVarInit();
+ auto trivialAutoVarInitSizeBound =
+ getContext().getLangOpts().TrivialAutoVarInitSizeBound;
CharUnits Size = getContext().getTypeSizeInChars(type);
bool isVolatile = type.isVolatileQualified();
if (!Size.isZero()) {
+auto allocSize =
CGM.getDataLayout().getTypeAllocSize(Loc.getElementType());
haopliu wrote:
Thank you both for clarifying! Moved the filtering to `emitStoresForConstant`
before memset.
Please take another look :-D
https://github.com/llvm/llvm-project/pull/74777
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Add size filter for stack auto init (PR #74777)
@@ -1759,20 +1759,29 @@ void
CodeGenFunction::emitZeroOrPatternForAutoVarInit(QualType type,
const VarDecl ,
Address Loc) {
auto trivialAutoVarInit = getContext().getLangOpts().getTrivialAutoVarInit();
+ auto trivialAutoVarInitSizeBound =
+ getContext().getLangOpts().TrivialAutoVarInitSizeBound;
CharUnits Size = getContext().getTypeSizeInChars(type);
bool isVolatile = type.isVolatileQualified();
if (!Size.isZero()) {
+auto allocSize =
CGM.getDataLayout().getTypeAllocSize(Loc.getElementType());
vitalybuka wrote:
I'd expect that it's kind of `bail out` measure which should not be consider as
a contract by a end-user. So confusing should not be a concern.
Also how
```
struct S {
long a;
long b;
char c;
} var;
```
is better then:
```
long a;
long b;
char c;
```
My impression was that your concern is buffers of hundreds of bytes, then maybe
better to target just them?
If it 's `24 vs 16` maybe it should not use auto init at all?
https://github.com/llvm/llvm-project/pull/74777
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Add size filter for stack auto init (PR #74777)
@@ -1759,20 +1759,29 @@ void
CodeGenFunction::emitZeroOrPatternForAutoVarInit(QualType type,
const VarDecl ,
Address Loc) {
auto trivialAutoVarInit = getContext().getLangOpts().getTrivialAutoVarInit();
+ auto trivialAutoVarInitSizeBound =
+ getContext().getLangOpts().TrivialAutoVarInitSizeBound;
CharUnits Size = getContext().getTypeSizeInChars(type);
bool isVolatile = type.isVolatileQualified();
if (!Size.isZero()) {
+auto allocSize =
CGM.getDataLayout().getTypeAllocSize(Loc.getElementType());
haopliu wrote:
`struct S {
long a;
long b;
char c;
}; S var;` The `var` size is 24 bytes. `emitStoresForConstant` splits the
constant store and emits several stores instead. Assume we set the max-size
flag to 16. If filter the size in `emitStoresForConstant`, this var will be
auto-initialized (each split store is less than 16) even though its size > 16.
Would it be confusing?
https://github.com/llvm/llvm-project/pull/74777
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Add size filter for stack auto init (PR #74777)
@@ -1759,20 +1759,29 @@ void
CodeGenFunction::emitZeroOrPatternForAutoVarInit(QualType type,
const VarDecl ,
Address Loc) {
auto trivialAutoVarInit = getContext().getLangOpts().getTrivialAutoVarInit();
+ auto trivialAutoVarInitSizeBound =
+ getContext().getLangOpts().TrivialAutoVarInitSizeBound;
CharUnits Size = getContext().getTypeSizeInChars(type);
bool isVolatile = type.isVolatileQualified();
if (!Size.isZero()) {
+auto allocSize =
CGM.getDataLayout().getTypeAllocSize(Loc.getElementType());
efriedma-quic wrote:
Ignoring a few less common cases (C++ classes with non-trivial constructors,
unions, self-referencing initializers), variable initialization is generally is
an all-or-nothing proposition. If you write something like `struct X { int
x,y,z; }; struct X x[100] = {0};`, that initializes every field of every
element. What examples are you worried about?
https://github.com/llvm/llvm-project/pull/74777
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Add size filter for stack auto init (PR #74777)
https://github.com/haopliu edited https://github.com/llvm/llvm-project/pull/74777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Add size filter for stack auto init (PR #74777)
@@ -1759,20 +1759,29 @@ void
CodeGenFunction::emitZeroOrPatternForAutoVarInit(QualType type,
const VarDecl ,
Address Loc) {
auto trivialAutoVarInit = getContext().getLangOpts().getTrivialAutoVarInit();
+ auto trivialAutoVarInitSizeBound =
+ getContext().getLangOpts().TrivialAutoVarInitSizeBound;
CharUnits Size = getContext().getTypeSizeInChars(type);
bool isVolatile = type.isVolatileQualified();
if (!Size.isZero()) {
+auto allocSize =
CGM.getDataLayout().getTypeAllocSize(Loc.getElementType());
haopliu wrote:
The problem of `emitStoresForConstant` is that it may split struct/array
variables to a handful of stores, which would mess up the size filtering (e.g.,
difficult to debug). What do you think?
> what if we have a large array of structs with a few uninitialized fields.
Initializing the rest will cost us anyway about the same.
(Correct me if I'm wrong) While emitting the auto-init, clang has no idea how
each filed/element is initialized. It's difficult to distinguish these two
cases in `CGDecl.cpp`:
- A large array of structs with all fields uninitialized.
- A large array of structs with a few field uninitialized.
https://github.com/llvm/llvm-project/pull/74777
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Add size filter for stack auto init (PR #74777)
@@ -656,6 +656,13 @@ def err_drv_trivial_auto_var_init_stop_after_missing_dependency : Error< def err_drv_trivial_auto_var_init_stop_after_invalid_value : Error< "'-ftrivial-auto-var-init-stop-after=*' only accepts positive integers">; +def err_drv_trivial_auto_var_init_size_bound_missing_dependency : Error< + "'-ftrivial-auto-var-init-size-bound=*' is used without " haopliu wrote: Thanks, done! https://github.com/llvm/llvm-project/pull/74777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Add size filter for stack auto init (PR #74777)
@@ -1759,20 +1759,29 @@ void
CodeGenFunction::emitZeroOrPatternForAutoVarInit(QualType type,
const VarDecl ,
Address Loc) {
auto trivialAutoVarInit = getContext().getLangOpts().getTrivialAutoVarInit();
+ auto trivialAutoVarInitSizeBound =
+ getContext().getLangOpts().TrivialAutoVarInitSizeBound;
CharUnits Size = getContext().getTypeSizeInChars(type);
bool isVolatile = type.isVolatileQualified();
if (!Size.isZero()) {
+auto allocSize =
CGM.getDataLayout().getTypeAllocSize(Loc.getElementType());
vitalybuka wrote:
After second thought
what if we have a large array of structs with a few uninitialized fields.
Initializing the rest will cost us anyway about the same.
Would it be better to check insize of
`emitStoresForZeroInit`/`emitStoresForPatternInit`, more precicely in
`emitStoresForConstant` before memset if IsAutoInit?
https://github.com/llvm/llvm-project/pull/74777
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Add size filter for stack auto init (PR #74777)
@@ -656,6 +656,13 @@ def err_drv_trivial_auto_var_init_stop_after_missing_dependency : Error< def err_drv_trivial_auto_var_init_stop_after_invalid_value : Error< "'-ftrivial-auto-var-init-stop-after=*' only accepts positive integers">; +def err_drv_trivial_auto_var_init_size_bound_missing_dependency : Error< + "'-ftrivial-auto-var-init-size-bound=*' is used without " vitalybuka wrote: other than naming LGTM https://github.com/llvm/llvm-project/pull/74777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Add size filter for stack auto init (PR #74777)
@@ -656,6 +656,13 @@ def err_drv_trivial_auto_var_init_stop_after_missing_dependency : Error< def err_drv_trivial_auto_var_init_stop_after_invalid_value : Error< "'-ftrivial-auto-var-init-stop-after=*' only accepts positive integers">; +def err_drv_trivial_auto_var_init_size_bound_missing_dependency : Error< + "'-ftrivial-auto-var-init-size-bound=*' is used without " vitalybuka wrote: size-bound -> max-size https://github.com/llvm/llvm-project/pull/74777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Add size filter for stack auto init (PR #74777)
https://github.com/haopliu edited https://github.com/llvm/llvm-project/pull/74777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
