Add debuginfo C support for a SetType, Subrangetype, dynamic array type and replace arrays #135607
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-debuginfo Author: peter mckinna (demoitem) ChangesThis change adds some support to the C DebugInfo capability to create set types, Full diff: https://github.com/llvm/llvm-project/pull/135607.diff 2 Files Affected:
diff --git a/llvm/include/llvm-c/DebugInfo.h b/llvm/include/llvm-c/DebugInfo.h
index 11e0b9b4c81e8..0f267d9941b08 100644
--- a/llvm/include/llvm-c/DebugInfo.h
+++ b/llvm/include/llvm-c/DebugInfo.h
@@ -673,7 +673,6 @@ LLVMMetadataRef LLVMDIBuilderCreateUnionType(
LLVMMetadataRef *Elements, unsigned NumElements, unsigned RunTimeLang,
const char *UniqueId, size_t UniqueIdLen);
-
/**
* Create debugging information entry for an array.
* \param Builder The DIBuilder.
@@ -689,6 +688,78 @@ LLVMDIBuilderCreateArrayType(LLVMDIBuilderRef Builder, uint64_t Size,
LLVMMetadataRef *Subscripts,
unsigned NumSubscripts);
+/**
+ * Create debugging information entry for a set.
+ * @param Builder The DIBuilder.
+ * \param Scope The scope this module is imported into.
+ * \param Name A name that uniquely identifies this set.
+ * \param NameLen The length of the C string passed to \c Name.
+ * \param File File where the set is located.
+ * \param Line Line number of the declaration.
+ * \param SizeInBits Set size.
+ * \param AlignInBits Set alignment.
+ * @param BaseTy The base type of the set.
+ */
+LLVMMetadataRef LLVMDIBuilderCreateSetType(
+ LLVMDIBuilderRef Builder, LLVMMetadataRef Scope, const char *Name,
+ size_t NameLen, LLVMMetadataRef File, unsigned LineNumber,
+ uint64_t SizeInBits, uint32_t AlignInBits, LLVMMetadataRef BaseTy);
+
+/**
+ * Create a descriptor for a subrange with dynamic bounds.
+ * \param Builder The DIBuilder.
+ * \param Scope The scope this module is imported into.
+ * \param Name A name that uniquely identifies this set.
+ * \param NameLen The length of the C string passed to \c Name.
+ * \param LineNo Line number.
+ * \param File File where the subrange is located.
+ * \param SizeInBits Member size.
+ * \param AlignInBits Member alignment.
+ * \param Flags Flags.
+ * \param BaseTy The base type of the subrange. eg integer or enumeration
+ * \param LowerBound Lower bound of the subrange.
+ * \param UpperBound Upper bound of the subrange.
+ * \param Stride Stride of the subrange.
+ * \param Bias Bias of the subrange.
+ */
+LLVMMetadataRef LLVMDIBuilderCreateSubrangeType(
+ LLVMDIBuilderRef Builder, LLVMMetadataRef Scope, const char *Name,
+ size_t NameLen, unsigned LineNo, LLVMMetadataRef File,
+ uint64_t SizeInBits, uint32_t AlignInBits,
+ LLVMDIFlags Flags, LLVMMetadataRef BaseTy,
+ LLVMMetadataRef LowerBound, LLVMMetadataRef UpperBound,
+ LLVMMetadataRef Stride, LLVMMetadataRef Bias);
+
+/**
+ * Create debugging information entry for a dynamic array.
+ * \param Builder The DIBuilder.
+ * \param Size Array size.
+ * \param AlignInBits Alignment.
+ * \param Ty Element type.
+ * \param Subscripts Subscripts.
+ * \param NumSubscripts Number of subscripts.
+ * \param DataLocation DataLocation.
+ * \param Associated Associated.
+ * \param Allocated Allocated.
+ * \param Rank Rank.
+ * \param BitStride BitStride.
+ */
+LLVMMetadataRef LLVMDIBuilderCreateDynamicArrayType(
+ LLVMDIBuilderRef Builder, LLVMMetadataRef Scope, const char *Name,
+ size_t NameLen, unsigned LineNo, LLVMMetadataRef File,
+ uint64_t Size, uint32_t AlignInBits,
+ LLVMMetadataRef Ty, LLVMMetadataRef *Subscripts, unsigned NumSubscripts,
+ LLVMMetadataRef DataLocation, LLVMMetadataRef Associated,
+ LLVMMetadataRef Allocated, LLVMMetadataRef Rank, LLVMMetadataRef BitStride);
+
+/**
+ * Replace arrays.
+ *
+ * @see DIBuilder::replaceArrays()
+ */
+void LLVMReplaceArrays(LLVMDIBuilderRef Builder, LLVMMetadataRef *T,
+ LLVMMetadataRef *Elements, unsigned NumElements);
+
/**
* Create debugging information entry for a vector type.
* \param Builder The DIBuilder.
diff --git a/llvm/lib/IR/DebugInfo.cpp b/llvm/lib/IR/DebugInfo.cpp
index cefd6fe14a3ac..c2f12f0efa030 100644
--- a/llvm/lib/IR/DebugInfo.cpp
+++ b/llvm/lib/IR/DebugInfo.cpp
@@ -1309,6 +1309,57 @@ return wrap(unwrap(Builder)->createEnumerationType(
LineNumber, SizeInBits, AlignInBits, Elts, unwrapDI<DIType>(ClassTy)));
}
+LLVMMetadataRef LLVMDIBuilderCreateSetType(
+ LLVMDIBuilderRef Builder, LLVMMetadataRef Scope, const char *Name,
+ size_t NameLen, LLVMMetadataRef File, unsigned LineNumber,
+ uint64_t SizeInBits, uint32_t AlignInBits, LLVMMetadataRef BaseTy) {
+ return wrap(unwrap(Builder)->createSetType(
+ unwrapDI<DIScope>(Scope), {Name, NameLen}, unwrapDI<DIFile>(File),
+ LineNumber, SizeInBits, AlignInBits, unwrapDI<DIType>(BaseTy)));
+}
+
+LLVMMetadataRef LLVMDIBuilderCreateSubrangeType(
+ LLVMDIBuilderRef Builder, LLVMMetadataRef Scope, const char *Name,
+ size_t NameLen, unsigned LineNo, LLVMMetadataRef File,
+ uint64_t SizeInBits, uint32_t AlignInBits,
+ LLVMDIFlags Flags, LLVMMetadataRef BaseTy,
+ LLVMMetadataRef LowerBound, LLVMMetadataRef UpperBound,
+ LLVMMetadataRef Stride, LLVMMetadataRef Bias
+ ) {
+ return wrap(unwrap(Builder)->createSubrangeType(
+ {Name, NameLen}, unwrapDI<DIFile>(File), LineNo,
+ unwrapDI<DIScope>(Scope), SizeInBits, AlignInBits,
+ map_from_llvmDIFlags(Flags), unwrapDI<DIType>(BaseTy),
+ unwrap<DIExpression>(LowerBound), unwrap<DIExpression>(UpperBound),
+ unwrap<DIExpression>(Stride), unwrap<DIExpression>(Bias)));
+}
+
+LLVMMetadataRef LLVMDIBuilderCreateDynamicArrayType(
+ LLVMDIBuilderRef Builder, LLVMMetadataRef Scope, const char *Name,
+ size_t NameLen, unsigned LineNo, LLVMMetadataRef File,
+ uint64_t Size, uint32_t AlignInBits,
+ LLVMMetadataRef Ty, LLVMMetadataRef *Subscripts, unsigned NumSubscripts,
+ LLVMMetadataRef DataLocation, LLVMMetadataRef Associated,
+ LLVMMetadataRef Allocated, LLVMMetadataRef Rank, LLVMMetadataRef BitStride) {
+ auto Subs =
+ unwrap(Builder)->getOrCreateArray({unwrap(Subscripts), NumSubscripts});
+ return wrap(unwrap(Builder)->createArrayType(
+ unwrapDI<DIScope>(Scope),
+ {Name, NameLen}, unwrapDI<DIFile>(File), LineNo,
+ Size, AlignInBits, unwrapDI<DIType>(Ty), Subs,
+ unwrap<DIExpression>(DataLocation), unwrap<DIExpression>(Associated),
+ unwrap<DIExpression>(Allocated), unwrap<DIExpression>(Rank),
+ unwrap(BitStride)));
+}
+
+void LLVMReplaceArrays(LLVMDIBuilderRef Builder, LLVMMetadataRef *T,
+ LLVMMetadataRef *Elements, unsigned NumElements) {
+ auto Arr = unwrap<DICompositeType>(*T);
+ auto Elts = unwrap(Builder)->getOrCreateArray({unwrap(Elements),
+ NumElements});
+ unwrap(Builder)->replaceArrays(Arr, Elts);
+}
+
LLVMMetadataRef LLVMDIBuilderCreateUnionType(
LLVMDIBuilderRef Builder, LLVMMetadataRef Scope, const char *Name,
size_t NameLen, LLVMMetadataRef File, unsigned LineNumber,
|
|
@llvm/pr-subscribers-llvm-ir Author: peter mckinna (demoitem) ChangesThis change adds some support to the C DebugInfo capability to create set types, Full diff: https://github.com/llvm/llvm-project/pull/135607.diff 2 Files Affected:
diff --git a/llvm/include/llvm-c/DebugInfo.h b/llvm/include/llvm-c/DebugInfo.h
index 11e0b9b4c81e8..0f267d9941b08 100644
--- a/llvm/include/llvm-c/DebugInfo.h
+++ b/llvm/include/llvm-c/DebugInfo.h
@@ -673,7 +673,6 @@ LLVMMetadataRef LLVMDIBuilderCreateUnionType(
LLVMMetadataRef *Elements, unsigned NumElements, unsigned RunTimeLang,
const char *UniqueId, size_t UniqueIdLen);
-
/**
* Create debugging information entry for an array.
* \param Builder The DIBuilder.
@@ -689,6 +688,78 @@ LLVMDIBuilderCreateArrayType(LLVMDIBuilderRef Builder, uint64_t Size,
LLVMMetadataRef *Subscripts,
unsigned NumSubscripts);
+/**
+ * Create debugging information entry for a set.
+ * @param Builder The DIBuilder.
+ * \param Scope The scope this module is imported into.
+ * \param Name A name that uniquely identifies this set.
+ * \param NameLen The length of the C string passed to \c Name.
+ * \param File File where the set is located.
+ * \param Line Line number of the declaration.
+ * \param SizeInBits Set size.
+ * \param AlignInBits Set alignment.
+ * @param BaseTy The base type of the set.
+ */
+LLVMMetadataRef LLVMDIBuilderCreateSetType(
+ LLVMDIBuilderRef Builder, LLVMMetadataRef Scope, const char *Name,
+ size_t NameLen, LLVMMetadataRef File, unsigned LineNumber,
+ uint64_t SizeInBits, uint32_t AlignInBits, LLVMMetadataRef BaseTy);
+
+/**
+ * Create a descriptor for a subrange with dynamic bounds.
+ * \param Builder The DIBuilder.
+ * \param Scope The scope this module is imported into.
+ * \param Name A name that uniquely identifies this set.
+ * \param NameLen The length of the C string passed to \c Name.
+ * \param LineNo Line number.
+ * \param File File where the subrange is located.
+ * \param SizeInBits Member size.
+ * \param AlignInBits Member alignment.
+ * \param Flags Flags.
+ * \param BaseTy The base type of the subrange. eg integer or enumeration
+ * \param LowerBound Lower bound of the subrange.
+ * \param UpperBound Upper bound of the subrange.
+ * \param Stride Stride of the subrange.
+ * \param Bias Bias of the subrange.
+ */
+LLVMMetadataRef LLVMDIBuilderCreateSubrangeType(
+ LLVMDIBuilderRef Builder, LLVMMetadataRef Scope, const char *Name,
+ size_t NameLen, unsigned LineNo, LLVMMetadataRef File,
+ uint64_t SizeInBits, uint32_t AlignInBits,
+ LLVMDIFlags Flags, LLVMMetadataRef BaseTy,
+ LLVMMetadataRef LowerBound, LLVMMetadataRef UpperBound,
+ LLVMMetadataRef Stride, LLVMMetadataRef Bias);
+
+/**
+ * Create debugging information entry for a dynamic array.
+ * \param Builder The DIBuilder.
+ * \param Size Array size.
+ * \param AlignInBits Alignment.
+ * \param Ty Element type.
+ * \param Subscripts Subscripts.
+ * \param NumSubscripts Number of subscripts.
+ * \param DataLocation DataLocation.
+ * \param Associated Associated.
+ * \param Allocated Allocated.
+ * \param Rank Rank.
+ * \param BitStride BitStride.
+ */
+LLVMMetadataRef LLVMDIBuilderCreateDynamicArrayType(
+ LLVMDIBuilderRef Builder, LLVMMetadataRef Scope, const char *Name,
+ size_t NameLen, unsigned LineNo, LLVMMetadataRef File,
+ uint64_t Size, uint32_t AlignInBits,
+ LLVMMetadataRef Ty, LLVMMetadataRef *Subscripts, unsigned NumSubscripts,
+ LLVMMetadataRef DataLocation, LLVMMetadataRef Associated,
+ LLVMMetadataRef Allocated, LLVMMetadataRef Rank, LLVMMetadataRef BitStride);
+
+/**
+ * Replace arrays.
+ *
+ * @see DIBuilder::replaceArrays()
+ */
+void LLVMReplaceArrays(LLVMDIBuilderRef Builder, LLVMMetadataRef *T,
+ LLVMMetadataRef *Elements, unsigned NumElements);
+
/**
* Create debugging information entry for a vector type.
* \param Builder The DIBuilder.
diff --git a/llvm/lib/IR/DebugInfo.cpp b/llvm/lib/IR/DebugInfo.cpp
index cefd6fe14a3ac..c2f12f0efa030 100644
--- a/llvm/lib/IR/DebugInfo.cpp
+++ b/llvm/lib/IR/DebugInfo.cpp
@@ -1309,6 +1309,57 @@ return wrap(unwrap(Builder)->createEnumerationType(
LineNumber, SizeInBits, AlignInBits, Elts, unwrapDI<DIType>(ClassTy)));
}
+LLVMMetadataRef LLVMDIBuilderCreateSetType(
+ LLVMDIBuilderRef Builder, LLVMMetadataRef Scope, const char *Name,
+ size_t NameLen, LLVMMetadataRef File, unsigned LineNumber,
+ uint64_t SizeInBits, uint32_t AlignInBits, LLVMMetadataRef BaseTy) {
+ return wrap(unwrap(Builder)->createSetType(
+ unwrapDI<DIScope>(Scope), {Name, NameLen}, unwrapDI<DIFile>(File),
+ LineNumber, SizeInBits, AlignInBits, unwrapDI<DIType>(BaseTy)));
+}
+
+LLVMMetadataRef LLVMDIBuilderCreateSubrangeType(
+ LLVMDIBuilderRef Builder, LLVMMetadataRef Scope, const char *Name,
+ size_t NameLen, unsigned LineNo, LLVMMetadataRef File,
+ uint64_t SizeInBits, uint32_t AlignInBits,
+ LLVMDIFlags Flags, LLVMMetadataRef BaseTy,
+ LLVMMetadataRef LowerBound, LLVMMetadataRef UpperBound,
+ LLVMMetadataRef Stride, LLVMMetadataRef Bias
+ ) {
+ return wrap(unwrap(Builder)->createSubrangeType(
+ {Name, NameLen}, unwrapDI<DIFile>(File), LineNo,
+ unwrapDI<DIScope>(Scope), SizeInBits, AlignInBits,
+ map_from_llvmDIFlags(Flags), unwrapDI<DIType>(BaseTy),
+ unwrap<DIExpression>(LowerBound), unwrap<DIExpression>(UpperBound),
+ unwrap<DIExpression>(Stride), unwrap<DIExpression>(Bias)));
+}
+
+LLVMMetadataRef LLVMDIBuilderCreateDynamicArrayType(
+ LLVMDIBuilderRef Builder, LLVMMetadataRef Scope, const char *Name,
+ size_t NameLen, unsigned LineNo, LLVMMetadataRef File,
+ uint64_t Size, uint32_t AlignInBits,
+ LLVMMetadataRef Ty, LLVMMetadataRef *Subscripts, unsigned NumSubscripts,
+ LLVMMetadataRef DataLocation, LLVMMetadataRef Associated,
+ LLVMMetadataRef Allocated, LLVMMetadataRef Rank, LLVMMetadataRef BitStride) {
+ auto Subs =
+ unwrap(Builder)->getOrCreateArray({unwrap(Subscripts), NumSubscripts});
+ return wrap(unwrap(Builder)->createArrayType(
+ unwrapDI<DIScope>(Scope),
+ {Name, NameLen}, unwrapDI<DIFile>(File), LineNo,
+ Size, AlignInBits, unwrapDI<DIType>(Ty), Subs,
+ unwrap<DIExpression>(DataLocation), unwrap<DIExpression>(Associated),
+ unwrap<DIExpression>(Allocated), unwrap<DIExpression>(Rank),
+ unwrap(BitStride)));
+}
+
+void LLVMReplaceArrays(LLVMDIBuilderRef Builder, LLVMMetadataRef *T,
+ LLVMMetadataRef *Elements, unsigned NumElements) {
+ auto Arr = unwrap<DICompositeType>(*T);
+ auto Elts = unwrap(Builder)->getOrCreateArray({unwrap(Elements),
+ NumElements});
+ unwrap(Builder)->replaceArrays(Arr, Elts);
+}
+
LLVMMetadataRef LLVMDIBuilderCreateUnionType(
LLVMDIBuilderRef Builder, LLVMMetadataRef Scope, const char *Name,
size_t NameLen, LLVMMetadataRef File, unsigned LineNumber,
|
OCHyams
left a comment
OCHyams
left a comment
There was a problem hiding this comment.
Hi @demoitem, thanks for the PR. This seems reasonable to me, but should probably be tested. Please could you test by adding calls to llvm/tools/llvm-c-test/debuginfo.c and updating the relevant checks in llvm/test/Bindings/llvm-c/debug_info_new_format.ll?
llvm/include/llvm-c/DebugInfo.h
Outdated
| /** | ||
| * Create debugging information entry for a set. | ||
| * @param Builder The DIBuilder. | ||
| * \param Scope The scope this module is imported into. |
There was a problem hiding this comment.
Is the Scope param description right - perhaps something like The scope in which the set is defined. makes more sense?. Same question for LLVMDIBuilderCreateSubrangeType.
There was a problem hiding this comment.
Okay changed that.
llvm/include/llvm-c/DebugInfo.h
Outdated
| * \param Line Line number of the declaration. | ||
| * \param SizeInBits Set size. | ||
| * \param AlignInBits Set alignment. | ||
| * @param BaseTy The base type of the set. |
There was a problem hiding this comment.
Nit: While we're here, for consistency please can we use \param instead of @param for BaseTy and Builder too?
There was a problem hiding this comment.
Yes fixed those as well.
|
Added that test case. Let me know if there is anything else. |
|
I'm just checking the status of this request. I see that the build failed but am at a loss to |
|
Yes without any backtrace it's not great: If you build locally you may be able to get the same error with a backtrace printed out as well. If it doesn't print out, attach a debugger to the test case and backtrace in there. You may need to add If you are using a debug build, then assertions are already enabled. (https://www.llvm.org/docs/CMake.html#llvm-enable-assertions) This assertion is to do with LLVM's own version of runtime type information, so something has tried to cast some object to type |
and for replacing arrays to the C inteface
and for replacing arrays to the C inteface
|
I wonder if someone could merge this change. I have built it against the latest main and it compiles fine. |
|
I can reproduce the failure locally with cmake config: If you use a debug build ( This is the backtrace: The To and From types: The cast is actually requested by this wrapper macro: And I have no idea what that's doing or why, but there are some ways you can investigate at least. Before merging you will least one approval before this can be merged. Perhaps @OCHyams has the time to review, but if not, someone from @llvm/pr-subscribers-debug-info will do so. |
OCHyams
left a comment
OCHyams
left a comment
There was a problem hiding this comment.
Sorry, this fell off my radar.
I can see that in DwarfUnit::constructSubrangeDIE DIExpression s are valid for lowerBound, upperBound, stride and bias... but I can't see any LLVM tests that exercise those. That's not something you need to address with this commit, but I thought it worth bringing up as I was going to ask "is this actually a supported configuration", and found that by digging about.
cc @tromey who looks to have added DISubrangeType initially in #126772 - would you be able to give this patch a quick review if you are able?
I think I found the cause of the assertion failure - please see my inline comments.
| * \param BaseTy The base type of the subrange. eg integer or enumeration | ||
| * \param LowerBound Lower bound of the subrange. | ||
| * \param UpperBound Upper bound of the subrange. | ||
| * \param Stride Stride of the subrange. | ||
| * \param Bias Bias of the subrange. |
There was a problem hiding this comment.
Given the assertion failure, I think more info about the expected type is needed here, and you should mention which parameters are ok to be null. Same for the other functions.
There was a problem hiding this comment.
Please could you address this comment?
There was a problem hiding this comment.
IMO it's still worth explaining which types that are accepted here for these various parameters. Unlike LLVMDIBuilderCreateDynamicArrayType they won't assert if the wrong type is passed in; they're checked by the verifier. So the user should still get a helpful message eventually, but it's not the best API-usage experience.
That said, none of the other C-API functions here look like they try very hard to explain the expected inputs, so it's not the end of the world to keep as is. However in future it's good practice to ensure you have responded to or actioned all comments (github does sometimes make it very easy to miss things!).
llvm/tools/llvm-c-test/debuginfo.c
Outdated
| LLVMMetadataRef lo = LLVMValueAsMetadata(FooVal1); | ||
| LLVMMetadataRef hi = LLVMValueAsMetadata(FooVal2); | ||
| LLVMMetadataRef SubrangeMetadataTy = LLVMDIBuilderCreateSubrangeType( | ||
| DIB, File, "foo", 3, 42, File, 64, 0, 0, Int64Ty, lo, hi, NULL, NULL); |
There was a problem hiding this comment.
Is the assertion caused by passing NULL in here? The C API call calls unwrap which calls llvm::cast, which asserts if given non-null pointers.
As @DavidSpickett points out, the test might pass locally for you if you're not building with assertions.
There was a problem hiding this comment.
Thanks for the quick response. My reading of the backtrace suggests it is the lowerbound which is
asserting. There seems to be a problem wrapping a DIExpression. I may have been hasty in treating those
parameters as expressions as the DIBuilder function treats them as either a constant, a variable or and
expression. So how is one supposed to wrap that? The strange thing is that without asserts it works and
generates the correct debug tags. Or am I completely wrong in that assessment.
Sorry about not adding tests for that. Expressions in this context are needed by Ada but I haven't yet implemented that in gnat-llvm. |
|
I think it's ready now. I fixed those wrapper problems and the builds completed ok. Just need someone |
|
Ping |
|
Just in case the comment gets lost (github has a habit of swallowing inline comments...) - there's an outstanding nit for the function comments (specifically for the argument descriptions):
|
|
Right. Will fix those. Just waiting for the build to finish. Takes a while, several days in fact on this laptop, Tends to overheat. |
|
I have modified the header to mention which parameters are not null and tested the rest which |
It seems a bit strange to require those in particular to be non-null, because they're really only useful for Fortran. |
|
Wondering if this is ok to merge now. |
|
I had another look after that query about Fortran and changed the unwrap to unwrapDI for those parameters |
I think the rule is that Like in the patch, the set type constructor uses On the other hand, the patch requires a non- Anyway, that's my understanding. Hopefully it's accurate, if not I'd appreciate a correction. |
|
I was just having a look and there's a third option (first on this list) -
It's not particularly clear at all, I was also a bit confused there trying to understand why
I've left a comment in line. I appreciate this has been a bit of a protracted review, sorry about that. If making those final changes is going to be difficult with your hardware I might be able to take it over, if that's helpful (can't promise when though). |
OCHyams
left a comment
OCHyams
left a comment
There was a problem hiding this comment.
(... actually post the inline comment...)
| LLVMMetadataRef LLVMDIBuilderCreateDynamicArrayType( | ||
| LLVMDIBuilderRef Builder, LLVMMetadataRef Scope, const char *Name, | ||
| size_t NameLen, unsigned LineNo, LLVMMetadataRef File, uint64_t Size, | ||
| uint32_t AlignInBits, LLVMMetadataRef Ty, LLVMMetadataRef *Subscripts, | ||
| unsigned NumSubscripts, LLVMMetadataRef DataLocation, | ||
| LLVMMetadataRef Associated, LLVMMetadataRef Allocated, LLVMMetadataRef Rank, | ||
| LLVMMetadataRef BitStride) { | ||
| auto Subs = | ||
| unwrap(Builder)->getOrCreateArray({unwrap(Subscripts), NumSubscripts}); | ||
| return wrap(unwrap(Builder)->createArrayType( | ||
| unwrapDI<DIScope>(Scope), {Name, NameLen}, unwrapDI<DIFile>(File), LineNo, | ||
| Size, AlignInBits, unwrapDI<DIType>(Ty), Subs, | ||
| unwrapDI<DIExpression>(DataLocation), unwrapDI<DIExpression>(Associated), | ||
| unwrapDI<DIExpression>(Allocated), unwrapDI<DIExpression>(Rank), | ||
| unwrap(BitStride))); | ||
| } |
There was a problem hiding this comment.
Is unwrapDI<DIExpression> is the right move for these parameters? createArrayType takes PointerUnion<DIExpression *, DIVariable *>, as it looks like it expects either
an expression or variable, but this implementation means only DIExpressions are accepted by the C interface.
Something like the code-suggestion below might be good to cover all inputs. Please can you update the function comment to reflect the accepted types, and update the test to check variables, expressions and null.
| LLVMMetadataRef LLVMDIBuilderCreateDynamicArrayType( | |
| LLVMDIBuilderRef Builder, LLVMMetadataRef Scope, const char *Name, | |
| size_t NameLen, unsigned LineNo, LLVMMetadataRef File, uint64_t Size, | |
| uint32_t AlignInBits, LLVMMetadataRef Ty, LLVMMetadataRef *Subscripts, | |
| unsigned NumSubscripts, LLVMMetadataRef DataLocation, | |
| LLVMMetadataRef Associated, LLVMMetadataRef Allocated, LLVMMetadataRef Rank, | |
| LLVMMetadataRef BitStride) { | |
| auto Subs = | |
| unwrap(Builder)->getOrCreateArray({unwrap(Subscripts), NumSubscripts}); | |
| return wrap(unwrap(Builder)->createArrayType( | |
| unwrapDI<DIScope>(Scope), {Name, NameLen}, unwrapDI<DIFile>(File), LineNo, | |
| Size, AlignInBits, unwrapDI<DIType>(Ty), Subs, | |
| unwrapDI<DIExpression>(DataLocation), unwrapDI<DIExpression>(Associated), | |
| unwrapDI<DIExpression>(Allocated), unwrapDI<DIExpression>(Rank), | |
| unwrap(BitStride))); | |
| } | |
| /// MD may be nullptr, a DIExpression or DIVariable. | |
| PointerUnion<DIExpression *, DIVariable *> unwrapExprVar(LLVMMetadataRef MD) { | |
| if (!MD) | |
| return nullptr; | |
| MDNode *MDN = unwrapDI<MDNode>(MD); | |
| if (auto *E = dyn_cast<DIExpression>(MDN)) | |
| return E; | |
| assert(isa<DIVariable>(MDN) && "Expected DIExpression or DIVariable"); | |
| return cast<DIVariable>(MDN); | |
| } | |
| LLVMMetadataRef LLVMDIBuilderCreateDynamicArrayType( | |
| LLVMDIBuilderRef Builder, LLVMMetadataRef Scope, const char *Name, | |
| size_t NameLen, unsigned LineNo, LLVMMetadataRef File, uint64_t Size, | |
| uint32_t AlignInBits, LLVMMetadataRef Ty, LLVMMetadataRef *Subscripts, | |
| unsigned NumSubscripts, LLVMMetadataRef DataLocation, | |
| LLVMMetadataRef Associated, LLVMMetadataRef Allocated, LLVMMetadataRef Rank, | |
| LLVMMetadataRef BitStride) { | |
| auto Subs = | |
| unwrap(Builder)->getOrCreateArray({unwrap(Subscripts), NumSubscripts}); | |
| return wrap(unwrap(Builder)->createArrayType( | |
| unwrapDI<DIScope>(Scope), {Name, NameLen}, unwrapDI<DIFile>(File), LineNo, | |
| Size, AlignInBits, unwrapDI<DIType>(Ty), Subs, | |
| unwrapExprVar(DataLocation), unwrapExprVar(Associated), | |
| unwrapExprVar(Allocated), unwrapExprVar(Rank), unwrap(BitStride))); | |
| } |
Thanks for the comment, I was unaware of the |
given to CreateDynamicArray. Plus additional tests to check.
|
I think it's getting closer. Made those changes, fixed the test and updated the comments. |
OCHyams
left a comment
OCHyams
left a comment
There was a problem hiding this comment.
I think this LGTM now, thanks
| * \param BaseTy The base type of the subrange. eg integer or enumeration | ||
| * \param LowerBound Lower bound of the subrange. | ||
| * \param UpperBound Upper bound of the subrange. | ||
| * \param Stride Stride of the subrange. | ||
| * \param Bias Bias of the subrange. |
There was a problem hiding this comment.
IMO it's still worth explaining which types that are accepted here for these various parameters. Unlike LLVMDIBuilderCreateDynamicArrayType they won't assert if the wrong type is passed in; they're checked by the verifier. So the user should still get a helpful message eventually, but it's not the best API-usage experience.
That said, none of the other C-API functions here look like they try very hard to explain the expected inputs, so it's not the end of the world to keep as is. However in future it's good practice to ensure you have responded to or actioned all comments (github does sometimes make it very easy to miss things!).
|
Thanks @OCHyams for all your help, I don't have commit access, could you merge this PR for me? |
|
Ping |
|
Sorry, I've been away on PTO. Merging now |
|
@demoitem Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
5 participants
This change adds some support to the C DebugInfo capability to create set types,
subrange types, dynamic array types and the ability to replace arrays.