PNaCl: Check we don't crash in -replace-ptrs-with-ints with odd metadata operands.

PNaCl: Check we don't crash in -replace-ptrs-with-ints with odd metadata operands.

When LLVM proper is merge with `pnacl-llvm`, ensure the recent metadata changes don't cause a regression in `pnacl-llvm`.

BUG=
R=jfb@chromium.org, jfb@chromium.com

Committed: https://chromium.googlesource.com/native_client/pnacl-llvm/+/47508eb89fce23d6a68caddfa808caf23729b550

[Richard Diamond, 2015-04-17 02:47:36]

Not sure if you're back yet JF, regardless I've already landed this patch on
Rust's fork so no rush.

[JF, 2015-04-20 23:05:43]

jfb@chromium.org changed reviewers:
+ jfb@chromium.org

[JF, 2015-04-20 23:05:44]

Sorry for the long response time, I just got back.

https://codereview.chromium.org/1085483003/diff/1/test/Transforms/NaCl/replac...
File test/Transforms/NaCl/replace-ptrs-with-ints.ll (right):

https://codereview.chromium.org/1085483003/diff/1/test/Transforms/NaCl/replac...
test/Transforms/NaCl/replace-ptrs-with-ints.ll:419: tail call void
@llvm.dbg.value(metadata !{i8* null}, i64 2, metadata !1, metadata !14)
Does this only occur for null values? Or are there other cases that can hit this
bug? I guess clang never emits llvm.dbg.value that's null, but Rust does?

Seems like a good fix, I just want to make sure the test covers all cases.

[Richard Diamond, 2015-04-22 14:04:57]

https://codereview.chromium.org/1085483003/diff/1/test/Transforms/NaCl/replac...
File test/Transforms/NaCl/replace-ptrs-with-ints.ll (right):

https://codereview.chromium.org/1085483003/diff/1/test/Transforms/NaCl/replac...
test/Transforms/NaCl/replace-ptrs-with-ints.ll:419: tail call void
@llvm.dbg.value(metadata !{i8* null}, i64 2, metadata !1, metadata !14)
On 2015/04/20 at 23:05:44, JF wrote:
> Does this only occur for null values? Or are there other cases that can hit
this bug? I guess clang never emits llvm.dbg.value that's null, but Rust does?
> 
> Seems like a good fix, I just want to make sure the test covers all cases.

It was found when building  `rust.pexe`. I'm not sure why, where, etc. It has
never happened before; Rust, as far as I know, doesn't generate it.

What other value types are there to handle? The list includes only Instructions
and Constants, right?

[JF, 2015-04-22 17:48:29]

Could you update the patch, it doesn't compile right now, probably some merge
conflict.

https://codereview.chromium.org/1085483003/diff/1/lib/Transforms/NaCl/Replace...
File lib/Transforms/NaCl/ReplacePtrsWithInts.cpp (right):

https://codereview.chromium.org/1085483003/diff/1/lib/Transforms/NaCl/Replace...
lib/Transforms/NaCl/ReplacePtrsWithInts.cpp:291: }
MDVal isn't a valid value here.

https://codereview.chromium.org/1085483003/diff/1/test/Transforms/NaCl/replac...
File test/Transforms/NaCl/replace-ptrs-with-ints.ll (right):

https://codereview.chromium.org/1085483003/diff/1/test/Transforms/NaCl/replac...
test/Transforms/NaCl/replace-ptrs-with-ints.ll:419: tail call void
@llvm.dbg.value(metadata !{i8* null}, i64 2, metadata !1, metadata !14)
On 2015/04/22 14:04:57, Richard Diamond wrote:
> On 2015/04/20 at 23:05:44, JF wrote:
> > Does this only occur for null values? Or are there other cases that can hit
> this bug? I guess clang never emits llvm.dbg.value that's null, but Rust does?
> > 
> > Seems like a good fix, I just want to make sure the test covers all cases.
> 
> It was found when building  `rust.pexe`. I'm not sure why, where, etc. It has
> never happened before; Rust, as far as I know, doesn't generate it.
> 
> What other value types are there to handle? The list includes only
Instructions
> and Constants, right?

I was thinking about undef too. Doesn't make much sense, but then again neither
does null ;-)

[Richard Diamond, 2015-05-08 18:26:48]

On 2015/04/22 at 17:48:29, jfb wrote:
> Could you update the patch, it doesn't compile right now, probably some merge
conflict.
> 
>
https://codereview.chromium.org/1085483003/diff/1/lib/Transforms/NaCl/Replace...
> File lib/Transforms/NaCl/ReplacePtrsWithInts.cpp (right):
> 
>
https://codereview.chromium.org/1085483003/diff/1/lib/Transforms/NaCl/Replace...
> lib/Transforms/NaCl/ReplacePtrsWithInts.cpp:291: }
> MDVal isn't a valid value here.

I've updated this patch to just add tests. It seems I jumped the gun w.r.t.
creating a patch; Rust's LLVM is recent enough to have some of the metadata
changes, which caused the original issue in the Rust-PNaCl-LLVM fork.
This patch will prevent this regression in the future, though.

> 
>
https://codereview.chromium.org/1085483003/diff/1/test/Transforms/NaCl/replac...
> File test/Transforms/NaCl/replace-ptrs-with-ints.ll (right):
> 
>
https://codereview.chromium.org/1085483003/diff/1/test/Transforms/NaCl/replac...
> test/Transforms/NaCl/replace-ptrs-with-ints.ll:419: tail call void
@llvm.dbg.value(metadata !{i8* null}, i64 2, metadata !1, metadata !14)
> On 2015/04/22 14:04:57, Richard Diamond wrote:
> > On 2015/04/20 at 23:05:44, JF wrote:
> > > Does this only occur for null values? Or are there other cases that can
hit
> > this bug? I guess clang never emits llvm.dbg.value that's null, but Rust
does?
> > > 
> > > Seems like a good fix, I just want to make sure the test covers all cases.
> > 
> > It was found when building  `rust.pexe`. I'm not sure why, where, etc. It
has
> > never happened before; Rust, as far as I know, doesn't generate it.
> > 
> > What other value types are there to handle? The list includes only
Instructions
> > and Constants, right?
> 
> I was thinking about undef too. Doesn't make much sense, but then again
neither does null ;-)

Ah. Fixed.

[JF, 2015-05-14 21:21:12]

lgtm

[JF, 2015-05-14 21:30:53]

Committed patchset #2 (id:20001) manually as
47508eb89fce23d6a68caddfa808caf23729b550 (presubmit successful).