[DI] Have createClassType create a class type.

[deadalnix]

I was wondering why my use of createClassType was generating structure reccords, and it turns out the code is wrong (or for some reason i don't understand the intended use of the API).

clang itself does not use that part of the API, so this flew under the radar.

[llvmbot]

@llvm/pr-subscribers-llvm-ir

Author: None (deadalnix)

Changes

I was wondering why my use of createClassType was generating structure reccords, and it turns out the code is wrong (or for some reason i don't understand the intended use of the API).

clang itself does not use that part of the API, so this flew under the radar.

---

Full diff: https://github.com/llvm/llvm-project/pull/102624.diff

1 Files Affected:

• (modified) llvm/lib/IR/DIBuilder.cpp (+1-1)

diff --git a/llvm/lib/IR/DIBuilder.cpp b/llvm/lib/IR/DIBuilder.cpp
index 30b79b6d6f60fd..0db82cdd6373c8 100644
--- a/llvm/lib/IR/DIBuilder.cpp
+++ b/llvm/lib/IR/DIBuilder.cpp
@@ -509,7 +509,7 @@ DICompositeType *DIBuilder::createClassType(
          "createClassType should be called with a valid Context");
 
   auto *R = DICompositeType::get(
-      VMContext, dwarf::DW_TAG_structure_type, Name, File, LineNumber,
+      VMContext, dwarf::DW_TAG_class_type, Name, File, LineNumber,
       getNonCompileUnitScope(Context), DerivedFrom, SizeInBits, AlignInBits,
       OffsetInBits, Flags, Elements, RunTimeLang, VTableHolder,
       cast_or_null<MDTuple>(TemplateParams), UniqueIdentifier);

[dwblaikie]

this'll need test coverage - but also, what does Clang do? (does it use a different API? Perhaps this API you're updating is unused/dead and that would help explain why it's incorrect - in which case removing it might be the better option?)

[deadalnix]

The code I'm updating here is used through the C API. I'm not sure what API clang uses, I'm not familiar with the clang codebase, but not this one, because it generates proper DW_TAG_class_type debug infos.

[deadalnix]

As for a tests, I'm going to see what I can do, but I would urge you to think about merging this patch nevertheless as it seems a bit counterproductive to allow the original code to be merged in without proper tests but with bugs, but then refusing to merge bu fixes because there is no test.

Nevertheless, if I find a way to do this, I will, as clearly, the lack of tests in the original version would have avoided this kind of nonsense to begin with.

[dwblaikie]

Yeah - if we didn't require test cases for fixes because the functionality was previously not tested - that'd mean not requiring any tests for any bug fixes, which would be weird, right? (we do require it, but sometimes we miss cases, or folks think it's trivial/doesn't need testing (I tend to not be aligned with that perspective, even simple things I tend to prefer having test coverage)

Because I was curious if this was ever correct/how it regressed - and seems it regressed here: e274180

There is some unit test coverage for DIBuilder in llvm/unittests/IR/IRBuilderTest.cpp - so that's probably the place to add some coverage for this function, for instance.

[deadalnix]

Considering it's a monster diff, not surprising something slept through.

Thanks for the pointers for writing the test.

[deadalnix]

I added a test to ensure various composite types are generated using the right tag.

[dwblaikie]

Looks great, thanks!