Page MenuHomePhabricator

[CodeView] Lower __restrict and other pointer qualifiers correctly
ClosedPublic

Authored by rnk on Feb 7 2018, 7:05 PM.

Details

Summary

Qualifiers on a pointer or reference type may apply to either the
pointee or the pointer itself. Consider 'const char *' and 'char *
const'. In the first example, the pointee data may not be modified
without casts, and in the second example, the pointer may not be updated
to point to new data.

In the general case, qualifiers are applied to types with LF_MODIFIER
records, which support the usual const and volatile qualifiers as well
as the __unaligned extension qualifier.

However, LF_POINTER records, which are used for pointers, references,
and member pointers, have flags for qualifiers applying to the
*pointer*. In fact, this is the only way to represent the restrict
qualifier, which can only apply to pointers, and cannot qualify regular
data types.

This patch causes LLVM to correctly fold 'const' and 'volatile' pointer
qualifiers into the pointer record, as well as adding support for
'__restrict' qualifiers in the same place.

Based on a patch from Aaron Smith

Diff Detail

Repository
rL LLVM

Event Timeline

asmith created this revision.Feb 7 2018, 7:05 PM
asmith updated this revision to Diff 133360.Feb 7 2018, 7:41 PM
zturner added a reviewer: rnk.Feb 8 2018, 2:12 PM

+rnk, in the future add him on anything MSABI specific.

s/msabi/codeview/

(I can review too btw, just would suggest adding him also)

zturner added inline comments.Feb 8 2018, 2:38 PM
include/llvm/DebugInfo/CodeView/CodeView.h
302 ↗(On Diff #133360)

Can you post a code fragment that I can compile with cl.exe to see this value of 0x0008 being emitted on an LF_MODIFIER CodeView record?

We already have a PointerOptions::Restrict which is a separate enum from ModifierOptions and corresponds to an LF_POINTER record. Are you sure this value is correct?

If I write this code:

struct Foo {
  int x;
};

void foo(Foo &__restrict f) {
  printf("%d", f.x);
}

Then cl generates an LF_POINTER record with the restrict option, but it doesn't generate a corresponding LF_MODIFIER record

$ llvm-pdbutil.exe dump -types restricttest.pdb | grep 0x104C
  0x104C | LF_STRUCTURE [size = 36] `Foo`
           referent = 0x104C, mode = ref, opts = restrict, kind = ptr32

$ llvm-pdbutil.exe dump -types restricttest.pdb | grep -A 1 LF_MODIFIER | grep 0x104C
  (no results)
lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
1268–1269 ↗(On Diff #133360)

Based on the above hypothesis, is it possible that you need to instead use lowerTypePointer here?

zturner added inline comments.Feb 8 2018, 2:41 PM
include/llvm/DebugInfo/CodeView/CodeView.h
302 ↗(On Diff #133360)

1 additional line of grep context for clarity:

$ llvm-pdbutil.exe dump -types restricttest.pdb | grep -B 1 0x104C
           options: has ctor / dtor | contains nested class | has unique name
  0x104C | LF_STRUCTURE [size = 36] `Foo`
--
  0x104D | LF_POINTER [size = 12]
           referent = 0x104C, mode = ref, opts = restrict, kind = ptr32
asmith updated this revision to Diff 133971.Feb 12 2018, 5:56 PM
asmith edited the summary of this revision. (Show Details)

I should probably defer to @rnk for the main content of the review here.

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
1514–1516 ↗(On Diff #133971)

If this is a solution, then do we still need the FIXME?

rnk commandeered this revision.Feb 26 2018, 11:47 AM
rnk edited reviewers, added: asmith; removed: rnk.

I patched this in to try to understand why the DenseMap changes were necessary. I don't think they are. I'll grab this and upload a new version in a bit.

rnk updated this revision to Diff 135969.Feb 26 2018, 1:55 PM
  • rewrite
rnk retitled this revision from [CodeView] Lower type for dwarf::DW_TAG_restrict_type type to [CodeView] Lower __restrict and other pointer qualifiers correctly.Feb 26 2018, 1:56 PM
rnk edited the summary of this revision. (Show Details)
rnk edited the summary of this revision. (Show Details)Feb 26 2018, 2:05 PM
rnk updated this revision to Diff 135974.Feb 26 2018, 2:06 PM
  • Remove dead parameters
  • Fix a null deref for 'const void'
Hui added a subscriber: Hui.Feb 26 2018, 3:19 PM
Hui added inline comments.
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
2142 ↗(On Diff #135974)

I am thinking without extra information for the method getTypeIndex, two different DIType, for example, int* and int* __restrict, that having the same Ty, will be hashed with only one key in the DenseMap. So what is returned by finding DenseMap will be whatever is hashed first. This might not be correct. If this thought is right, changing DenseMap is still needed.

rnk added inline comments.Feb 26 2018, 3:51 PM
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
2142 ↗(On Diff #135974)

The DITypeRef key should encode this information already. In the first case, the key will be a single DW_TAG_pointer_type DIDerivedType node. In the second case, it will be a DW_TAG_restrict_type node wrapping a pointer node.

Hui added inline comments.Feb 26 2018, 4:31 PM
llvm/test/DebugInfo/COFF/type-quals.ll
13 ↗(On Diff #135974)

Could you check the type of p in the test? Should be something like

LocalSym {

  Kind: S_LOCAL (0x113E)
  Type: const int* (0x100E)
  Flags [ (0x0)
  ]
  VarName: p
}
rnk marked an inline comment as done.Feb 27 2018, 1:59 PM
rnk added inline comments.
llvm/lib/DebugInfo/CodeView/RecordName.cpp
170–175 ↗(On Diff #135974)

I noticed that these should be printed on the right like __restrict. That affects the type-quals.ll test. See also https://llvm.org/pr36541.

rnk updated this revision to Diff 136150.Feb 27 2018, 2:00 PM
  • Fix pointer type printing
This revision was not accepted when it landed; it landed in state Needs Review.Feb 27 2018, 2:11 PM
This revision was automatically updated to reflect the committed changes.