[PATCH] D81678: Introduce frozen attribute at call sites for stricter poison analysis

2020-06-25 Thread Gui Andrade via Phabricator via cfe-commits
guiand marked an inline comment as done.
guiand added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2082
+  const Type *RetTyPtr = RetTy.getTypePtr();
+  if (!RetTy->isVoidType() && !RetTyPtr->isRecordType() &&
+  RetAI.getKind() != ABIArgInfo::Indirect) {

rsmith wrote:
> Other types that we should think about from a padding perspective:
> 
>  * `nullptr_t` (which has the same size and alignment as `void*` but is all 
> padding)
>  * 80-bit `long double` and `_Complex long double` (I don't know whether we 
> guarantee to initialize the 6 padding bytes)
>  * member pointers might contain padding under some ABI rules -- under the MS 
> ABI, you get a struct containing N pointers followed by M ints, which could 
> have 4 bytes of tail padding on 64-bit targets
>  * vector types with tail padding (eg, a vector of 3 `char`s is sometimes 
> passed as an `i32` with one byte `undef`)
>  * matrix types (presumably the same concerns as for vector types apply here)
>  * maybe Obj-C object types?
>  * `_ExtInt` types (eg, returning an `_ExtInt(65)` initialized to 0 produces 
> an `{i64, i64}` containing 7 bytes of `undef`)
> 
> It would be safer to list exactly those types for which we know this 
> assumption is correct rather than assuming that it's correct by default -- I 
> think more than half of the `Type` subclasses for concrete canonical types 
> can have undef bits in their IR representation.
This is a good point, and I think there was some nuance that I had previously 
included in determining this for records (since removed) that I didn't think to 
include in CGCall.cpp.

I think one particular check, `llvm::DataLayout::typeSizeEqualsStoreSize`, 
handles a lot of these cases:
- `long double`
- oddly-sized vectors
- `_ExtInt` types

I don't know if the `matrix` type has internal padding (like structs) but if 
not it would cover that as well.  
I believe that the struct with padding wouldn't be an issue as we're excluding 
record types here.  
And I'd have to take a closer look at `nullptr_t` to see how it behaves here.

~~~

But if I'd like to build an exhaustive list of types I think will work 
correctly with a check like this:
- scalars
- floating point numbers
- pointers

I think I'd need also need an inner `typeSizeEqualsStoreSize` check on the base 
type of vectors/complex numbers.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81678/new/

https://reviews.llvm.org/D81678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D81678: Introduce frozen attribute at call sites for stricter poison analysis

2020-06-25 Thread Richard Smith - zygoloid via Phabricator via cfe-commits
rsmith added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2082
+  const Type *RetTyPtr = RetTy.getTypePtr();
+  if (!RetTy->isVoidType() && !RetTyPtr->isRecordType() &&
+  RetAI.getKind() != ABIArgInfo::Indirect) {

Other types that we should think about from a padding perspective:

 * `nullptr_t` (which has the same size and alignment as `void*` but is all 
padding)
 * 80-bit `long double` and `_Complex long double` (I don't know whether we 
guarantee to initialize the 6 padding bytes)
 * member pointers might contain padding under some ABI rules -- under the MS 
ABI, you get a struct containing N pointers followed by M ints, which could 
have 4 bytes of tail padding on 64-bit targets
 * vector types with tail padding (eg, a vector of 3 `char`s is sometimes 
passed as an `i32` with one byte `undef`)
 * matrix types (presumably the same concerns as for vector types apply here)
 * maybe Obj-C object types?
 * `_ExtInt` types (eg, returning an `_ExtInt(65)` initialized to 0 produces an 
`{i64, i64}` containing 7 bytes of `undef`)

It would be safer to list exactly those types for which we know this assumption 
is correct rather than assuming that it's correct by default -- I think more 
than half of the `Type` subclasses for concrete canonical types can have undef 
bits in their IR representation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81678/new/

https://reviews.llvm.org/D81678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D81678: Introduce frozen attribute at call sites for stricter poison analysis

2020-06-25 Thread Johannes Doerfert via Phabricator via cfe-commits
jdoerfert added a comment.

> We don't have any optimizations that infer either "speculatable" or "frozen", 
> though, so I'm not sure there's any practical impact here.

`speculatable` is on the shortlist for the Attributor. Will happen eventually. 
I am sure `frozen` will follow once it is in. We already came across the "need" 
in the `nonnull` conversation for example.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81678/new/

https://reviews.llvm.org/D81678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D81678: Introduce frozen attribute at call sites for stricter poison analysis

2020-06-23 Thread Eli Friedman via Phabricator via cfe-commits
efriedma added a comment.

In D81678#2109059 , @nlopes wrote:

> I'm a bit concerned with this patch as it increases the amount of UB that 
> LLVM exploits without any study of the impact.
>  For example, right now it's ok do this with clang (not with constants; make 
> it less trivial so clang doesn't fold it right away):
>
>   int f() { return INT_MAX + 1; }
>
> While technically this is UB in C, when lowered to LLVM IR, this function 
> returns poison.
>  When the frozen attribute is attached, the function will now trigger UB in 
> LLVM IR as well.
>  Is this what we want? It would be worthwhile to at least compile a few 
> programs to check if they break.


Even if we say that it's undefined behavior, we don't have to start converting 
"ret undef" to "unreachable".  I mean, it's something we could consider doing, 
but we don't have to immediately start doing it just because the attribute 
exists.  I expect that just hooking up the attribute to 
isGuaranteedNotToBeUndefOrPoison() will have almost no immediate effect.

> Also, what's the plan to detect these cases in ubsan?

I don't think this has any practical impact on our goals with sanitizers.  We 
should detect undefined behavior before it gets to the point of actually 
passing or returning an undef or poison value.

> Note that pure function calls with 'frozen' arguments become harder to hoist 
> from loops, for example. Since now calling this pure function can trigger UB 
> if one of the arguments is poison. You would need to introduce frozen 
> beforehand. I don't see this issue addressed in this patch.

A pure function can have undefined behavior, in general.

I guess the interaction between "speculatable" and "frozen" is a little weird.  
We don't have any optimizations that infer either "speculatable" or "frozen", 
though, so I'm not sure there's any practical impact here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81678/new/

https://reviews.llvm.org/D81678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D81678: Introduce frozen attribute at call sites for stricter poison analysis

2020-06-23 Thread Evgenii Stepanov via Phabricator via cfe-commits
eugenis added a comment.

>> Also, what's the plan to detect these cases in ubsan?
> 
> I don't think this has any practical impact on our goals with sanitizers.  We 
> should detect undefined behavior before it gets to the point of actually 
> passing or returning an undef or poison value.

MSan will take advantage of this by validating no-undef at runtime before the 
function call. This will detect more bugs, as well as detect existing bugs 
earlier, making them easier to reason about.

I'm not sure how we could use this information in ubsan.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81678/new/

https://reviews.llvm.org/D81678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D81678: Introduce frozen attribute at call sites for stricter poison analysis

2020-06-23 Thread Johannes Doerfert via Phabricator via cfe-commits
jdoerfert added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:4095
+}
+  }
   call->setCallingConv(getRuntimeCC());

eugenis wrote:
> guiand wrote:
> > jdoerfert wrote:
> > > Why would we do this? Function attributes are valid at the call site, no 
> > > need to copy them.
> > Do you mean that for some definition: `define @foo(i32 frozen %a, i32 
> > frozen %b)`, it's valid to issue a call instruction like `call @foo(i32 %a, 
> > i32 %b)` and its operands will be correctly identified as `frozen`? That's 
> > the kind of behavior I was seeing and I wasn't sure if it was an error.
> If you see CallBase::paramHasAttr, function definition attributes will be 
> taken into account when available.
> 
> I'm not sure what happens for indirect calls that do not have a Callee - we 
> need to make sure that the frontend emits the callsite attributes that match 
> the signature of the call.
> If you see CallBase::paramHasAttr, function definition attributes will be 
> taken into account when available.

Yes :)

> I'm not sure what happens for indirect calls that do not have a Callee - we 
> need to make sure that the frontend emits the callsite attributes that match 
> the signature of the call.

This would not trigger for indirect calls, `f` would be `nullptr`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81678/new/

https://reviews.llvm.org/D81678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D81678: Introduce frozen attribute at call sites for stricter poison analysis

2020-06-23 Thread Johannes Doerfert via Phabricator via cfe-commits
jdoerfert added a comment.

In D81678#2109059 , @nlopes wrote:

> I'm a bit concerned with this patch as it increases the amount of UB that 
> LLVM exploits without any study of the impact.
>  For example, right now it's ok do this with clang (not with constants; make 
> it less trivial so clang doesn't fold it right away):
>
>   int f() { return INT_MAX + 1; }
>
> While technically this is UB in C, when lowered to LLVM IR, this function 
> returns poison.
>  When the frozen attribute is attached, the function will now trigger UB in 
> LLVM IR as well.
>  Is this what we want?


I think this is at least a mode we want to support, yes. I would try to make 
this default if we have proper tooling in place.

> It would be worthwhile to at least compile a few programs to check if they 
> break. Also, what's the plan to detect these cases in ubsan? We shouldn't be 
> exploiting new UB without having good warnings/checks in place IMO.

+1, good points, especially the ubsan one.

> Note that pure function calls with 'frozen' arguments become harder to hoist 
> from loops, for example. Since now calling this pure function can trigger UB 
> if one of the arguments is poison. You would need to introduce frozen 
> beforehand. I don't see this issue addressed in this patch.

TBH, I don't think this is the case. Pure is not sufficient to hoist if it was 
not executed at all, only `speculatable` is. For a `speculatable` call you 
should be able to remove `frozen` from the attributes if you can't prove it. 
Actually, it might not be a good idea to have `speculatable` and `frozen` on 
the same call, given that they kinda contradict each other. Back to pure, aka 
`readnone` (I would assume is what you meant): If it is pure you cannot hoist 
it as it could cause UB  (`return 1/arg;` or `return 1 / 0;` for all we know.). 
Generally, I guess the return value attribute `frozen` can be dropped though.

> For msan & friends, this patch makes sense, as they operate in a 
> rely-guarantee way. Initialization of memory is checked, so it can be relied 
> upon by the compiler later on.

At least the `nopoison` part is something we need in the IR so we can make 
violated value attributes cause poison and not UB, e.g., if you pass `NULL` to 
a `nonnull` argument.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81678/new/

https://reviews.llvm.org/D81678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D81678: Introduce frozen attribute at call sites for stricter poison analysis

2020-06-23 Thread Nuno Lopes via Phabricator via cfe-commits
nlopes added a comment.

I'm a bit concerned with this patch as it increases the amount of UB that LLVM 
exploits without any study of the impact.
For example, right now it's ok do this with clang (not with constants; make it 
less trivial so clang doesn't fold it right away):

  int f() { return INT_MAX + 1; }

While technically this is UB in C, when lowered to LLVM IR, this function 
returns poison.
When the frozen attribute is attached, the function will now trigger UB in LLVM 
IR as well.
Is this what we want? It would be worthwhile to at least compile a few programs 
to check if they break. Also, what's the plan to detect these cases in ubsan? 
We shouldn't be exploiting new UB without having good warnings/checks in place 
IMO.

Note that pure function calls with 'frozen' arguments become harder to hoist 
from loops, for example. Since now calling this pure function can trigger UB if 
one of the arguments is poison. You would need to introduce frozen beforehand. 
I don't see this issue addressed in this patch.

For msan & friends, this patch makes sense, as they operate in a rely-guarantee 
way. Initialization of memory is checked, so it can be relied upon by the 
compiler later on.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81678/new/

https://reviews.llvm.org/D81678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D81678: Introduce frozen attribute at call sites for stricter poison analysis

2020-06-23 Thread Gui Andrade via Phabricator via cfe-commits
guiand marked 2 inline comments as done.
guiand added inline comments.



Comment at: clang/include/clang/Driver/CC1Options.td:507
+def disable_frozen_args : Flag<["-"], "disable-frozen-args">,
+  HelpText<"Disable emitting frozen attribute in LLVM IR">;
 def load : Separate<["-"], "load">, MetaVarName<"">,

jdoerfert wrote:
> Do we want a second flag for return values or one to disable everything? 
> Having the ability to disable it partially seems odd.
The attribute is supposed to be a stopgap while not all the tests are up to 
date. It turns out that the return values are much less complicated to update 
the call operands, so a flag for disabling return position attributes wasn't 
necessary.



Comment at: clang/lib/CodeGen/CGCall.cpp:4095
+}
+  }
   call->setCallingConv(getRuntimeCC());

jdoerfert wrote:
> Why would we do this? Function attributes are valid at the call site, no need 
> to copy them.
Do you mean that for some definition: `define @foo(i32 frozen %a, i32 frozen 
%b)`, it's valid to issue a call instruction like `call @foo(i32 %a, i32 %b)` 
and its operands will be correctly identified as `frozen`? That's the kind of 
behavior I was seeing and I wasn't sure if it was an error.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81678/new/

https://reviews.llvm.org/D81678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D81678: Introduce frozen attribute at call sites for stricter poison analysis

2020-06-23 Thread Evgenii Stepanov via Phabricator via cfe-commits
eugenis added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:4095
+}
+  }
   call->setCallingConv(getRuntimeCC());

guiand wrote:
> jdoerfert wrote:
> > Why would we do this? Function attributes are valid at the call site, no 
> > need to copy them.
> Do you mean that for some definition: `define @foo(i32 frozen %a, i32 frozen 
> %b)`, it's valid to issue a call instruction like `call @foo(i32 %a, i32 %b)` 
> and its operands will be correctly identified as `frozen`? That's the kind of 
> behavior I was seeing and I wasn't sure if it was an error.
If you see CallBase::paramHasAttr, function definition attributes will be taken 
into account when available.

I'm not sure what happens for indirect calls that do not have a Callee - we 
need to make sure that the frontend emits the callsite attributes that match 
the signature of the call.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81678/new/

https://reviews.llvm.org/D81678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D81678: Introduce frozen attribute at call sites for stricter poison analysis

2020-06-23 Thread Johannes Doerfert via Phabricator via cfe-commits
jdoerfert added inline comments.



Comment at: clang/include/clang/Driver/CC1Options.td:507
+def disable_frozen_args : Flag<["-"], "disable-frozen-args">,
+  HelpText<"Disable emitting frozen attribute in LLVM IR">;
 def load : Separate<["-"], "load">, MetaVarName<"">,

Do we want a second flag for return values or one to disable everything? Having 
the ability to disable it partially seems odd.



Comment at: clang/lib/CodeGen/CGCall.cpp:4095
+}
+  }
   call->setCallingConv(getRuntimeCC());

Why would we do this? Function attributes are valid at the call site, no need 
to copy them.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81678/new/

https://reviews.llvm.org/D81678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D81678: Introduce frozen attribute at call sites for stricter poison analysis

2020-06-22 Thread Gui Andrade via Phabricator via cfe-commits
guiand added a comment.

I've added the test change to yet another diff, which you can find here: 
https://reviews.llvm.org/D82317


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81678/new/

https://reviews.llvm.org/D81678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D81678: Introduce frozen attribute at call sites for stricter poison analysis

2020-06-22 Thread Gui Andrade via Phabricator via cfe-commits
guiand added a comment.

@jdoerfert I've separated out changes to the language reference to 
https://reviews.llvm.org/D82316 as you suggested. I kept the name `frozen` for 
now while we reach a consensus regarding its final name.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81678/new/

https://reviews.llvm.org/D81678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D81678: Introduce frozen attribute at call sites for stricter poison analysis

2020-06-22 Thread Gui Andrade via Phabricator via cfe-commits
guiand updated this revision to Diff 272489.
guiand added a comment.

I've updated this patch to only include the actual implementation of `frozen`, 
for easier review.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81678/new/

https://reviews.llvm.org/D81678

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/CC1Options.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  llvm/include/llvm/Bitcode/LLVMBitCodes.h
  llvm/include/llvm/IR/Attributes.td
  llvm/lib/AsmParser/LLLexer.cpp
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/AsmParser/LLToken.h
  llvm/lib/Bitcode/Reader/BitcodeReader.cpp
  llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/lib/IR/Attributes.cpp
  llvm/lib/Transforms/Utils/CodeExtractor.cpp

Index: llvm/lib/Transforms/Utils/CodeExtractor.cpp
===
--- llvm/lib/Transforms/Utils/CodeExtractor.cpp
+++ llvm/lib/Transforms/Utils/CodeExtractor.cpp
@@ -929,6 +929,7 @@
   case Attribute::StrictFP:
   case Attribute::UWTable:
   case Attribute::NoCfCheck:
+  case Attribute::Frozen:
 break;
   }
 
Index: llvm/lib/IR/Attributes.cpp
===
--- llvm/lib/IR/Attributes.cpp
+++ llvm/lib/IR/Attributes.cpp
@@ -443,6 +443,8 @@
 return "cold";
   if (hasAttribute(Attribute::ImmArg))
 return "immarg";
+  if (hasAttribute(Attribute::Frozen))
+return "frozen";
 
   if (hasAttribute(Attribute::ByVal)) {
 std::string Result;
Index: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
===
--- llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
+++ llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
@@ -731,6 +731,8 @@
 return bitc::ATTR_KIND_SANITIZE_MEMTAG;
   case Attribute::Preallocated:
 return bitc::ATTR_KIND_PREALLOCATED;
+  case Attribute::Frozen:
+return bitc::ATTR_KIND_FROZEN;
   case Attribute::EndAttrKinds:
 llvm_unreachable("Can not encode end-attribute kinds marker.");
   case Attribute::None:
Index: llvm/lib/Bitcode/Reader/BitcodeReader.cpp
===
--- llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -1532,6 +1532,8 @@
 return Attribute::SanitizeMemTag;
   case bitc::ATTR_KIND_PREALLOCATED:
 return Attribute::Preallocated;
+  case bitc::ATTR_KIND_FROZEN:
+return Attribute::Frozen;
   }
 }
 
Index: llvm/lib/AsmParser/LLToken.h
===
--- llvm/lib/AsmParser/LLToken.h
+++ llvm/lib/AsmParser/LLToken.h
@@ -196,6 +196,7 @@
   kw_naked,
   kw_nest,
   kw_noalias,
+  kw_frozen,
   kw_nobuiltin,
   kw_nocapture,
   kw_noduplicate,
Index: llvm/lib/AsmParser/LLParser.cpp
===
--- llvm/lib/AsmParser/LLParser.cpp
+++ llvm/lib/AsmParser/LLParser.cpp
@@ -1374,6 +1374,7 @@
 case lltok::kw_inalloca:
 case lltok::kw_nest:
 case lltok::kw_noalias:
+case lltok::kw_frozen:
 case lltok::kw_nocapture:
 case lltok::kw_nonnull:
 case lltok::kw_returned:
@@ -1677,6 +1678,9 @@
 case lltok::kw_inalloca:B.addAttribute(Attribute::InAlloca); break;
 case lltok::kw_inreg:   B.addAttribute(Attribute::InReg); break;
 case lltok::kw_nest:B.addAttribute(Attribute::Nest); break;
+case lltok::kw_frozen:
+  B.addAttribute(Attribute::Frozen);
+  break;
 case lltok::kw_noalias: B.addAttribute(Attribute::NoAlias); break;
 case lltok::kw_nocapture:   B.addAttribute(Attribute::NoCapture); break;
 case lltok::kw_nofree:  B.addAttribute(Attribute::NoFree); break;
@@ -1774,6 +1778,9 @@
 }
 case lltok::kw_inreg:   B.addAttribute(Attribute::InReg); break;
 case lltok::kw_noalias: B.addAttribute(Attribute::NoAlias); break;
+case lltok::kw_frozen:
+  B.addAttribute(Attribute::Frozen);
+  break;
 case lltok::kw_nonnull: B.addAttribute(Attribute::NonNull); break;
 case lltok::kw_signext: B.addAttribute(Attribute::SExt); break;
 case lltok::kw_zeroext: B.addAttribute(Attribute::ZExt); break;
Index: llvm/lib/AsmParser/LLLexer.cpp
===
--- llvm/lib/AsmParser/LLLexer.cpp
+++ llvm/lib/AsmParser/LLLexer.cpp
@@ -696,6 +696,7 @@
   KEYWORD(writeonly);
   KEYWORD(zeroext);
   KEYWORD(immarg);
+  KEYWORD(frozen);
 
   KEYWORD(type);
   KEYWORD(opaque);
Index: llvm/include/llvm/IR/Attributes.td
===
--- llvm/include/llvm/IR/Attributes.td
+++ llvm/include/llvm/IR/Attributes.td
@@ -39,6 +39,9 @@
 /// Pass structure by value.
 def ByVal : TypeAttr<"byval">;
 
+/// Parameter or return value may not contain 

[PATCH] D81678: Introduce frozen attribute at call sites for stricter poison analysis

2020-06-21 Thread Juneyoung Lee via Phabricator via cfe-commits
aqjune added a comment.

In D81678#2105077 , @eugenis wrote:

> > Could you explicitly state that if it is aggregate or vector type all 
> > elements and paddings should be frozen too?
>
> If an aggregate is passed as an aggregate at IR level, we should not care 
> about the padding.
>  Unless it's coerced to an integer.


Oh sorry, I missed the comment of @guiand above.
I wanted to suggest making things simpler by having its memory representation 
always have well-defined bits if stored into memory.
But, if it is helpful for MSan's performance because it is well-defined in C, I 
think it is fine to not care about the paddings.
I suggest explicitly mentioning that we're not considering paddings of 
aggregates in the LangRef (at the definition of frozen value too if you want; 
actually I thought it was already described in the definition, but it didn't).

In D81678#2105429 , @jdoerfert wrote:

> I'm unsure if we want the name `frozen` as it is less helpful to anyone now 
> familiar with the frozen instruction.
>  In a different thread we concluded that we need some sort of `nopoison` as 
> an attribute to convey the behavior is UB if the value would be poison.
>  I would very much prefer a self explanatory spelling here, especially since 
> `nopoison` will be derived from sources other than the frozen instruction.


In the nonnull thread I was in favor of `nopoison` as well, but became to think 
that `nopoison` is a bit misleading in this patch because it doesn't talk about 
undef bit.
Personally I preferred `frozen` because whenever there is an ambiguity in its 
semantics, the precise definition can be made by simply updating the notion of 
a frozen value, but if a more intuitive spelling is consideration I'm open to 
any suggestions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81678/new/

https://reviews.llvm.org/D81678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D81678: Introduce frozen attribute at call sites for stricter poison analysis

2020-06-21 Thread Johannes Doerfert via Phabricator via cfe-commits
jdoerfert requested changes to this revision.
jdoerfert added a comment.
This revision now requires changes to proceed.

First, apologies for being late, I didn't properly monitor the list recently.

---

This diff is impossible to review and later to understand.

I would ask you to split it, at least for review purposes, such that (1) lang 
ref changes, (2) code changes, (3) test changes are separate.
Please also upload it with a full diff (at least for (1) and (2)).

The revision title needs to be updated (maybe add `[LangRef]` or `[Attr]`), and 
the commit message needs more explanation of the reasons and semantics.

---

Lang Ref (https://reviews.llvm.org/differential/changeset/?ref=2023449 <- so 
people can find it)

I think the spelling is not in line with other attributes and needs to be 
changed.

I'm unsure if we want the name `frozen` as it is less helpful to anyone now 
familiar with the frozen instruction.
In a different thread we concluded that we need some sort of `nopoison` as an 
attribute to convey the behavior is UB if the value would be poison.
I would very much prefer a self explanatory spelling here, especially since 
`nopoison` will be derived from sources other than the frozen instruction.

In the lang ref this is listed with and shown as string attribute. It should be 
with the regular ones (as it is implemented as such). It is also not target 
specific.

---

All in all we need to open this up to more people and go over the lang ref 
changes again.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81678/new/

https://reviews.llvm.org/D81678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D81678: Introduce frozen attribute at call sites for stricter poison analysis

2020-06-20 Thread Juneyoung Lee via Phabricator via cfe-commits
aqjune accepted this revision.
aqjune added a comment.
This revision is now accepted and ready to land.

For the LangRef part, LGTM modulo the suggested change. Could anyone review 
clang and LLVM's updated part?




Comment at: llvm/docs/LangRef.rst:1644
+and return values. When ``"frozen"`` is present, every bit of the
+tagged type is fully initialized and never poison.
 ``"probe-stack"``

Could you explicitly state that if it is aggregate or vector type all elements 
and paddings should be frozen too?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81678/new/

https://reviews.llvm.org/D81678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D81678: Introduce frozen attribute at call sites for stricter poison analysis

2020-06-20 Thread Gui Andrade via Phabricator via cfe-commits
guiand updated this revision to Diff 272213.
guiand retitled this revision from "Introduce partialinit attribute at call 
sites for stricter poison analysis" to "Introduce frozen attribute at call 
sites for stricter poison analysis".
guiand edited the summary of this revision.
guiand added a comment.
Herald added subscribers: sstefan1, phosek, dmgreen, mstorsjo, arphaman, 
dylanmckay, dschuff, emaste.
Herald added a reviewer: jdoerfert.

Reversing the meaning of the attribute to `frozen` and having the code not 
apply it to records removed pretty much all the logical changes in the code.

I took Juneyoung's suggestion and added a new cc1 flag, -disable-frozen-args 
(doesn't apply to return position, only argument position `frozen` attributes), 
and applied that to some 150 particularly problematic tests. Thousands of other 
tests were programatically and manually changed to include the new attribute.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81678/new/

https://reviews.llvm.org/D81678

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/CC1Options.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/ARCMT/objcmt-instancetype.m
  clang/test/ARCMT/objcmt-instancetype.m.result
  clang/test/ARCMT/objcmt-numeric-literals.m
  clang/test/ARCMT/objcmt-numeric-literals.m.result
  clang/test/AST/ast-dump-openmp-begin-declare-variant_6.c
  clang/test/ASTMerge/unnamed_fields/test.cpp
  clang/test/Analysis/casts.c
  clang/test/Analysis/inlining/DynDispatchBifurcate.m
  clang/test/Analysis/inlining/InlineObjCClassMethod.m
  clang/test/Analysis/misc-ps-region-store.m
  clang/test/Analysis/missing-bind-temporary.cpp
  clang/test/Analysis/silence-checkers-and-packages-core-all.cpp
  clang/test/CXX/class/class.compare/class.compare.default/p4.cpp
  clang/test/CXX/dcl.dcl/basic.namespace/namespace.udecl/p11.cpp
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/dtor.cpp
  clang/test/CXX/dcl.decl/dcl.init/dcl.init.ref/p5-var.cpp
  clang/test/CXX/dcl.decl/dcl.meaning/dcl.fct/p2-cxx0x.cpp
  clang/test/CXX/drs/dr0xx.cpp
  clang/test/CXX/except/except.spec/p14-ir.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/blocks-irgen.mm
  clang/test/CXX/modules-ts/codegen-basics.cppm
  clang/test/CXX/special/class.copy/p3.cpp
  clang/test/CodeGen/2004-02-13-Memset.c
  clang/test/CodeGen/2004-06-17-UnorderedCompares.c
  clang/test/CodeGen/2006-05-19-SingleEltReturn.c
  clang/test/CodeGen/2007-02-25-C-DotDotDot.c
  clang/test/CodeGen/2007-06-18-SextAttrAggregate.c
  clang/test/CodeGen/2008-03-05-syncPtr.c
  clang/test/CodeGen/2008-07-29-override-alias-decl.c
  clang/test/CodeGen/2008-07-30-implicit-initialization.c
  clang/test/CodeGen/2008-07-31-promotion-of-compound-pointer-arithmetic.c
  clang/test/CodeGen/2009-02-13-zerosize-union-field.c
  clang/test/CodeGen/2009-05-04-EnumInreg.c
  clang/test/CodeGen/2009-09-24-SqrtErrno.c
  clang/test/CodeGen/3dnow-builtins.c
  clang/test/CodeGen/64bit-swiftcall.c
  clang/test/CodeGen/PR3589-freestanding-libcalls.c
  clang/test/CodeGen/_Bool-conversion.c
  clang/test/CodeGen/aapcs-align.cpp
  clang/test/CodeGen/aapcs64-align.cpp
  clang/test/CodeGen/aarch64-args.cpp
  clang/test/CodeGen/aarch64-byval-temp.c
  clang/test/CodeGen/aarch64-neon-3v.c
  clang/test/CodeGen/aarch64-neon-across.c
  clang/test/CodeGen/aarch64-neon-dot-product.c
  clang/test/CodeGen/aarch64-neon-extract.c
  clang/test/CodeGen/aarch64-neon-fcvt-intrinsics.c
  clang/test/CodeGen/aarch64-neon-fma.c
  clang/test/CodeGen/aarch64-neon-ldst-one.c
  clang/test/CodeGen/aarch64-neon-scalar-copy.c
  clang/test/CodeGen/aarch64-neon-scalar-x-indexed-elem.c
  clang/test/CodeGen/aarch64-neon-tbl.c
  clang/test/CodeGen/aarch64-neon-vcombine.c
  clang/test/CodeGen/aarch64-neon-vget-hilo.c
  clang/test/CodeGen/aarch64-neon-vget.c
  clang/test/CodeGen/aarch64-poly128.c
  clang/test/CodeGen/aarch64-poly64.c
  clang/test/CodeGen/aarch64-varargs-ms.c
  clang/test/CodeGen/aarch64-varargs.c
  clang/test/CodeGen/address-space-avr.c
  clang/test/CodeGen/address-space-field1.c
  clang/test/CodeGen/address-space.c
  clang/test/CodeGen/aggregate-assign-call.c
  clang/test/CodeGen/aix-return.c
  clang/test/CodeGen/aix-struct-arg.c
  clang/test/CodeGen/aix-vaargs.c
  clang/test/CodeGen/alias.c
  clang/test/CodeGen/align-param.c
  clang/test/CodeGen/align_value.cpp
  clang/test/CodeGen/alloc-align-attr.c
  clang/test/CodeGen/arc/arguments.c
  clang/test/CodeGen/arc/struct-align.c
  clang/test/CodeGen/arm-aapcs-vfp.c
  clang/test/CodeGen/arm-abi-vector.c
  clang/test/CodeGen/arm-arguments.c
  clang/test/CodeGen/arm-bf16-params-returns.c
  clang/test/CodeGen/arm-byval-align.c
  clang/test/CodeGen/arm-cmse-attr.c
  clang/test/CodeGen/arm-cmse-call.c
  clang/test/CodeGen/arm-float-helpers.c
  clang/test/CodeGen/arm-fp16-arguments.c
  clang/test/CodeGen/arm-homogenous.c
  clang/test/CodeGen/arm-mangle-bf16.cpp

[PATCH] D81678: Introduce frozen attribute at call sites for stricter poison analysis

2020-06-20 Thread Evgenii Stepanov via Phabricator via cfe-commits
eugenis added a comment.

> Could you explicitly state that if it is aggregate or vector type all 
> elements and paddings should be frozen too?

If an aggregate is passed as an aggregate at IR level, we should not care about 
the padding.
Unless it's coerced to an integer.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81678/new/

https://reviews.llvm.org/D81678



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits