Page MenuHomePhabricator

[clang] initialize type qualifiers for FunctionNoProtoType
ClosedPublic

Authored by rmaz on Sep 9 2022, 10:01 AM.

Details

Summary

When initializing FunctionNoProtoType types, zero out the type
qualifiers. This will ensure the ODR hash remains stable as it
hashes the values for these qualifiers for all function types.

Event Timeline

rmaz created this revision.Sep 9 2022, 10:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2022, 10:01 AM
rmaz requested review of this revision.Sep 9 2022, 10:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2022, 10:01 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rmaz edited the summary of this revision. (Show Details)Sep 9 2022, 10:03 AM
rmaz updated this revision to Diff 459108.Sep 9 2022, 10:12 AM

Move code to VisitFunctionProtoType instead of branch

rmaz edited the summary of this revision. (Show Details)Sep 9 2022, 10:20 AM
rmaz added reviewers: drodriguez, rtrieu, mikael.
rtrieu added a comment.Sep 9 2022, 1:30 PM

Do you have a test case for this?

rmaz added a comment.Sep 9 2022, 2:00 PM

Do you have a test case for this?

Was struggling to think of a good one. How about a test that repeatedly generates a pcm for a func decl with no params and checks if the DECL_FUNCTION record is the same?

rtrieu added a comment.Sep 9 2022, 2:40 PM

Do you have a test case for this?

Was struggling to think of a good one. How about a test that repeatedly generates a pcm for a func decl with no params and checks if the DECL_FUNCTION record is the same?

Have you looked at clang/test/Modules/odr_hash.cpp? It's where most of the ODR hash testing takes place by testing that Decls can be merged properly instead of checking the contents of pcm files.. Using #if define, it creates multiple modules from the same file. I would suggest creating two functions in each of the modules, then in the main file, using the function to force it to be loaded from the modules and merged together. The test should fail with the current Clang, but pass with your patch. You may need to create your test file if you need different compiler options.

rmaz planned changes to this revision.Sep 12 2022, 2:48 PM

I think it is probably safer to zero out the FastTypeQuals instead.

rmaz updated this revision to Diff 459884.Sep 13 2022, 2:55 PM

zero out qual types in constructor

rmaz retitled this revision from [clang] do not hash undefined qualifiers for FunctionNoProtoType to [clang] initialize type qualifiers for FunctionNoProtoType.Sep 13 2022, 2:56 PM
rmaz edited the summary of this revision. (Show Details)
rmaz added a comment.Sep 13 2022, 3:00 PM

Do you have a test case for this?

Was struggling to think of a good one. How about a test that repeatedly generates a pcm for a func decl with no params and checks if the DECL_FUNCTION record is the same?

Have you looked at clang/test/Modules/odr_hash.cpp? It's where most of the ODR hash testing takes place by testing that Decls can be merged properly instead of checking the contents of pcm files.. Using #if define, it creates multiple modules from the same file. I would suggest creating two functions in each of the modules, then in the main file, using the function to force it to be loaded from the modules and merged together. The test should fail with the current Clang, but pass with your patch. You may need to create your test file if you need different compiler options.

I took a look at writing a test to cover this, but hit the following problem: function qualifiers are only valid on c++ members, and there we cannot create FunctionNoProtoTypes. I couldn't think of a way of testing this by comparing module output that would fail deterministically, as it relies on reusing uninitialized memory that is non-zero.

rmaz updated this revision to Diff 461689.Sep 20 2022, 1:52 PM
rmaz edited the summary of this revision. (Show Details)

Add unit test for type qualifiers

rmaz updated this revision to Diff 461912.Sep 21 2022, 8:57 AM

Specify target and language for test

I took a look at writing a test to cover this, but hit the following problem: function qualifiers are only valid on c++ members, and there we cannot create FunctionNoProtoTypes. I couldn't think of a way of testing this by comparing module output that would fail deterministically, as it relies on reusing uninitialized memory that is non-zero.

This is why I'm wondering how we're hitting this problem in the first place. C++ shouldn't be able to create a function without a prototype so why does the ODR hash matter (do we use that in C and I just wasn't aware of it)?

clang/include/clang/AST/Type.h
3950

It seems a bit odd to me that we only want to initialize one member of the bits and none of the rest.

clang/unittests/AST/DeclTest.cpp
365

You should also pass -std=c89 explicitly so that the test continues to work when we switch the default language mode to C23 someday in the future.

rmaz added a comment.Sep 22 2022, 1:06 PM

This is why I'm wondering how we're hitting this problem in the first place. C++ shouldn't be able to create a function without a prototype so why does the ODR hash matter (do we use that in C and I just wasn't aware of it)?

The ODR hash matters because it is serialized in PCM output, regardless of which function type it is:

https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/ODRHash.cpp#L889-L908

Leaving the FastTypeQuals uninitialized can lead to non-deterministic values of the ODR hash, and so the serialized module output. With enough leading 0s this can lead to a size change in the PCM file, which will start to fail builds due to a size mismatch during PCM validation.

Some alternative approaches could be:

  • don't call these methods in VisitFunctionNoProtoType (and audit other callsites)
  • remove these methods from FunctionNoProtoType
rmaz added inline comments.Sep 22 2022, 1:12 PM
clang/include/clang/AST/Type.h
3950

The reason I only set the FastTypeQuals is because the rest of the FunctionTypeBits are not accessed in the base class or this class, except for ExtInfo which is initialized in the base class.

rmaz updated this revision to Diff 462275.Sep 22 2022, 1:13 PM

add -std=c89 to test

rmaz marked an inline comment as done.Sep 22 2022, 1:14 PM
rtrieu added inline comments.Sep 22 2022, 9:18 PM
clang/include/clang/AST/Type.h
3950

This feels error-prone since any new classes derived from FunctionType will need to also have this.

I think the safer change is to modify FunctionType::getFastTypeQuals(). If this is a FunctionProtoType, use FastTypeQuals to create a Qualifiers. If it is not, return a default created Qualifiers. This way, we won't need to increase the access to data members.

rmaz updated this revision to Diff 462483.Sep 23 2022, 7:48 AM

Return default Qualifiers for FunctionNoProtoType

rmaz marked an inline comment as done.Sep 23 2022, 7:49 AM
rmaz updated this revision to Diff 462518.Sep 23 2022, 9:24 AM

Remove unnecessary friend class

aaron.ballman accepted this revision.Sep 23 2022, 11:58 AM

LGTM aside from a minor coding style nit, thank you!

clang/include/clang/AST/Type.h
3898–3899
This revision is now accepted and ready to land.Sep 23 2022, 11:58 AM
rmaz updated this revision to Diff 462898.Sep 26 2022, 7:20 AM

remove else case

This revision was landed with ongoing or failed builds.Sep 26 2022, 9:49 AM
This revision was automatically updated to reflect the committed changes.

How correct is it to access isConst, isVolatile, isRestrict for FunctionNoProtoType? Yes, we can provide some default value but I'm curious if accessing that default value is correct.

For the record, I've tried to fix the same problem in https://reviews.llvm.org/D104963 in a different way.

rmaz added a comment.Oct 3 2022, 1:03 PM

How correct is it to access isConst, isVolatile, isRestrict for FunctionNoProtoType? Yes, we can provide some default value but I'm curious if accessing that default value is correct.

For the record, I've tried to fix the same problem in https://reviews.llvm.org/D104963 in a different way.

That was my initial solution as well, but it seemed safer to ensure these methods always returned a consistent value without auditing all the possible call sites. I agree that it doesn't seem right that these methods are on the base class at all if they are only valid in one of the subclasses.

Thanks for the explanation, it is good to know the reasons for the current direction. Though it still feels like fixing a symptom and not the root cause.

How correct is it to access isConst, isVolatile, isRestrict for FunctionNoProtoType? Yes, we can provide some default value but I'm curious if accessing that default value is correct.

For the record, I've tried to fix the same problem in https://reviews.llvm.org/D104963 in a different way.

That was my initial solution as well, but it seemed safer to ensure these methods always returned a consistent value without auditing all the possible call sites. I agree that it doesn't seem right that these methods are on the base class at all if they are only valid in one of the subclasses.

The trouble is -- FunctionNoProtoType is pretty rare to encounter and is getting more rare by the year now that C has removed the feature entirely, but you can't build a prototyped function from an unprototyped one or vice versa (their only relationship is that they're both function types with a known return type). So if we pushed the methods from FunctionType down into FunctionProtoType, we'd have to run through the casting machinery a lot more than we currently do which could potentially regress compile times for very little benefit.

Personally, I think the next step is to add a local assert() to this function to try to find out why we're calling this on functions without a prototype and fix up the call sites. I think the intent is that you should not be calling this function on an unprototyped function and it'd be good for debug builds to yell if we're doing that, but returning a default constructed object is a safe recovery for release builds.

Personally, I think the next step is to add a local assert() to this function to try to find out why we're calling this on functions without a prototype and fix up the call sites. I think the intent is that you should not be calling this function on an unprototyped function and it'd be good for debug builds to yell if we're doing that, but returning a default constructed object is a safe recovery for release builds.

The problem with that approach is it won't work with the unit test we have. When we have more ODR hashing in C, we can replace the unit test with the one in D104963 but I wasn't able to make that one fail reliably (no MSAN on macOS), so not sure it is a sufficient testing.