This is an archive of the discontinued LLVM Phabricator instance.

Propagate SourceLocations through to get a Loc on float_cast_overflow
ClosedPublic

Authored by filcab on Aug 5 2015, 1:08 AM.

Details

Summary

float_cast_overflow is the only UBSan check without a source location attached.
This patch propagates SourceLocations where necessary to get them to the
EmitCheck() call.

Diff Detail

Repository
rL LLVM

Event Timeline

filcab updated this revision to Diff 31337.Aug 5 2015, 1:08 AM
filcab retitled this revision from to Propagate SourceLocations through to get a Loc on float_cast_overflow.
filcab updated this object.
filcab added reviewers: rsmith, ABataev, rjmccall.
filcab added a subscriber: cfe-commits.
samsonov edited edge metadata.Aug 5 2015, 12:42 PM

Thank you for working on this! This change looks good to me, but I'll let Richard say a final word.

Note that you would need to make a corresponding change in UBSan compiler-rt runtime. Also, this is an ABI-breaking change - e.g. shared objects built with older Clang will not work properly with new runtime. I would recommend you to add *some* diagnostic to ubsan runtime - e.g. if it tries to read SourceLocation from FloatCastOverflowData passed to handler, but it looks more like TypeDescriptor, than it would crash with a verbose message.

filcab added a comment.Aug 5 2015, 2:13 PM

Note that you would need to make a corresponding change in UBSan compiler-rt runtime.

I finished this late last night, so I figured I'd upload the bigger patch and start getting comments. :-)

Also, this is an ABI-breaking change - e.g. shared objects built with older Clang will not work properly with new runtime. I would recommend you to add *some* diagnostic to ubsan runtime - e.g. if it tries to read SourceLocation from FloatCastOverflowData passed to handler, but it looks more like TypeDescriptor, than it would crash with a verbose message.

I've made the simple UBSan patch and am only missing changing tests (on my machine we do get file names and lines for those source locations, from debug info, but I'll figure out a way to differentiate those).
I'll add a small check for v1/v2 of FloatCastOverflowData.

I will only commit after both patches are accepted, too.

rsmith edited edge metadata.Aug 5 2015, 2:37 PM

Looks good to me too (though please hold off on this until you and samsonov have agreed on what to do about the ubsan ABI change).

lib/CodeGen/CGOpenMPRuntime.cpp
1288 ↗(On Diff #31337)

Please reorder the parameters to put the source location before the body, so that the expected lambda parameter is last (see http://llvm.org/docs/CodingStandards.html#format-lambdas-like-blocks-of-code).

filcab updated this revision to Diff 31427.Aug 5 2015, 11:25 PM
filcab marked an inline comment as done.
filcab edited edge metadata.

Address Richard's comments

Marking Richard's comment as done.

filcab updated this revision to Diff 31429.Aug 5 2015, 11:51 PM

Rebasing off of current trunk.

compiler-rt patch posted at http://reviews.llvm.org/D11793

This revision was automatically updated to reflect the committed changes.