This is an archive of the discontinued LLVM Phabricator instance.

[BPF] annotate DIType metadata for builtin preseve_array_access_index()
AbandonedPublic

Authored by yonghong-song on Aug 1 2019, 3:57 PM.

Details

Reviewers
ast
Summary

Previously, debuginfo types are annotated to
IR builtin preserve_struct_access_index() and
preserve_union_access_index(), but not
preserve_array_access_index(). The debug info
is useful to identify the root type name which
later will be used for type comparison.

For user access without explicit type conversions,
the previous scheme works as we can ignore intermediate
compiler generated type conversions (e.g., from union types to
union members) and still generate correct access index string.

The issue comes with user explicit type conversions, e.g.,
converting an array to a structure like below:

struct t { int a; char b[40]; };
struct p { int c; int d; };
struct t *var = ...;
... __builtin_preserve_access_index(&(((struct p *)&(var->b[0]))->d)) ...

Although BPF backend can derive the type of &(var->b[0]),
explicit type annotation make checking more consistent
and less error prone.

Another benefit is for multiple dimension array handling.
For example,

struct p { int c; int d; } g[8][9][10];
... __builtin_preserve_access_index(&g[2][3][4].d) ...

It would be possible to calculate the number of "struct p"'s
before accessing its member "d" if array debug info is
available as it contains each dimension range.

This patch enables to annotate IR builtin preserve_array_access_index()
with either debuginfo array type or pointer type,
depending on user expression.

Diff Detail

Repository
rC Clang

Event Timeline

yonghong-song created this revision.Aug 1 2019, 3:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2019, 3:57 PM
ast added inline comments.Aug 1 2019, 4:03 PM
lib/CodeGen/CGExpr.cpp
3402–3403

would it make sense to reorder and make it 'QualType *arrayType = nullptr', so only explicit pointers would be passed and the rest will get 'nullptr' automatically?

The corresponding IR intrinsic interface change is at https://reviews.llvm.org/D65617
The BPF backend to utilize this information is at https://reviews.llvm.org/D65618

yonghong-song marked an inline comment as done.Aug 1 2019, 4:09 PM
yonghong-song added inline comments.
lib/CodeGen/CGExpr.cpp
3402–3403

Yes, that makes sense. Previous I thought about grouping related fields together. But make nullptr as the default is a good idea.

reorder parameter arrayType to make it with default nullptr and simplifies the code.

ast accepted this revision.Aug 1 2019, 5:04 PM

and the diff is shorter now :)

This revision is now accepted and ready to land.Aug 1 2019, 5:04 PM
yonghong-song abandoned this revision.Aug 2 2019, 10:41 AM

abandon this one. use git monorepo https://reviews.llvm.org/D65664 since we have llvm/clang inter-dependence here.