Modules, redeclaration chains, and locally-scoped extern declarations

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Modules, redeclaration chains, and locally-scoped extern declarations

Kristof Beyls via cfe-dev
Hi, modules folks and cfe-dev. I think I've discovered a bug in how module loading handles redeclaration merging, but it might also be by design. (And honestly "by design" would be nice in my particular use case.) Here it is:

// ModuleA.h
void test() {
  extern void sharedFunc(void);
}

// ModuleB.h
void sharedFunc(int);

// client.c
#import <ModuleA.h>
#import <ModuleB.h>
// or the other order, for a slightly different experiment

In this situation, the locally-scoped declaration of 'sharedFunc' in ModuleA isn't resolved with the declaration in ModuleB. There's even a conflict in their types! Is this by design, or an oversight, or what?

One of the consequences of this is that in client.c's context, asking for the canonical decl for 'sharedFunc' gives me the declaration in ModuleB. From client.c's perspective, that's more useful than giving me the one in ModuleA, but it's not consistent with the behavior with modules turned off.

Any insights to offer? Should I file this at bugs.llvm.org?
Thanks,
Jordan

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Modules, redeclaration chains, and locally-scoped extern declarations

Kristof Beyls via cfe-dev
On Thu, 26 Sep 2019 at 14:38, Jordan Rose via cfe-dev <[hidden email]> wrote:
Hi, modules folks and cfe-dev. I think I've discovered a bug in how module loading handles redeclaration merging, but it might also be by design. (And honestly "by design" would be nice in my particular use case.) Here it is:

// ModuleA.h
void test() {
  extern void sharedFunc(void);
}

// ModuleB.h
void sharedFunc(int);

// client.c
#import <ModuleA.h>
#import <ModuleB.h>
// or the other order, for a slightly different experiment

In this situation, the locally-scoped declaration of 'sharedFunc' in ModuleA isn't resolved with the declaration in ModuleB. There's even a conflict in their types! Is this by design, or an oversight, or what?

Well.. we only merge declarations that have matching types. This decision predates my involvement, but I think it makes some sense: we probably don't want the deserialization code somewhat nondeterministically producing diagnostics when it notices problems like this, and deferring the diagnostic to an ambiguity error when the function is actually used seems like the most consistent user experience. And that ambiguity error will presumably not appear for a call to sharedFunc(), because (non-redeclaration) lookup for sharedFunc won't find the local extern declaration in 'test'. (But I would not be surprised to see an error if you attempt to redeclare sharedFunc after importing both modules, or if you try to build both headers as part of the same module.)

Regardless of whether we accept it, the above program is ill-formed (undefined in C / ill-formed with no diagnostic required in C++), so I would encourage you to limit your reliance on it. I don't think we want to guarantee that such code keeps compiling.

One of the consequences of this is that in client.c's context, asking for the canonical decl for 'sharedFunc' gives me the declaration in ModuleB. From client.c's perspective, that's more useful than giving me the one in ModuleA, but it's not consistent with the behavior with modules turned off.

Any insights to offer? Should I file this at bugs.llvm.org?

We generally don't produce eager diagnostics for conflicts between imported modules, and that's a feature: we want module import to be constant-time, which means we can't diagnose such things eagerly. So for the above program (with no use of sharedFunc), I don't think we should produce a diagnostic.

I think the current behavior is largely by design.

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Modules, redeclaration chains, and locally-scoped extern declarations

Kristof Beyls via cfe-dev


On Sep 26, 2019, at 14:50, Richard Smith <[hidden email]> wrote:

On Thu, 26 Sep 2019 at 14:38, Jordan Rose via cfe-dev <[hidden email]> wrote:
Hi, modules folks and cfe-dev. I think I've discovered a bug in how module loading handles redeclaration merging, but it might also be by design. (And honestly "by design" would be nice in my particular use case.) Here it is:

// ModuleA.h
void test() {
  extern void sharedFunc(void);
}

// ModuleB.h
void sharedFunc(int);

// client.c
#import <ModuleA.h>
#import <ModuleB.h>
// or the other order, for a slightly different experiment

In this situation, the locally-scoped declaration of 'sharedFunc' in ModuleA isn't resolved with the declaration in ModuleB. There's even a conflict in their types! Is this by design, or an oversight, or what?

Well.. we only merge declarations that have matching types. This decision predates my involvement, but I think it makes some sense: we probably don't want the deserialization code somewhat nondeterministically producing diagnostics when it notices problems like this, and deferring the diagnostic to an ambiguity error when the function is actually used seems like the most consistent user experience. And that ambiguity error will presumably not appear for a call to sharedFunc(), because (non-redeclaration) lookup for sharedFunc won't find the local extern declaration in 'test'. (But I would not be surprised to see an error if you attempt to redeclare sharedFunc after importing both modules, or if you try to build both headers as part of the same module.)

Regardless of whether we accept it, the above program is ill-formed (undefined in C / ill-formed with no diagnostic required in C++), so I would encourage you to limit your reliance on it. I don't think we want to guarantee that such code keeps compiling.

One of the consequences of this is that in client.c's context, asking for the canonical decl for 'sharedFunc' gives me the declaration in ModuleB. From client.c's perspective, that's more useful than giving me the one in ModuleA, but it's not consistent with the behavior with modules turned off.

Any insights to offer? Should I file this at bugs.llvm.org?

We generally don't produce eager diagnostics for conflicts between imported modules, and that's a feature: we want module import to be constant-time, which means we can't diagnose such things eagerly. So for the above program (with no use of sharedFunc), I don't think we should produce a diagnostic.

I think the current behavior is largely by design.

Thanks for the explanation. There's still weirdness here even if they do have the same types, since the canonical decl is going to be different, and that can have user-visible effects if, say, the local declaration of 'sharedFunc' has an attribute like warn_unused_result. Is that by design? (I made the types different to be provocative and to make the problem obvious.)

I know module loading does make an effort to merge redeclaration chains normally; it's only locally-scoped extern declarations that are being left out.

Jordan


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Modules, redeclaration chains, and locally-scoped extern declarations

Kristof Beyls via cfe-dev
On Thu, 26 Sep 2019 at 15:08, Jordan Rose via cfe-dev <[hidden email]> wrote:
On Sep 26, 2019, at 14:50, Richard Smith <[hidden email]> wrote:

On Thu, 26 Sep 2019 at 14:38, Jordan Rose via cfe-dev <[hidden email]> wrote:
Hi, modules folks and cfe-dev. I think I've discovered a bug in how module loading handles redeclaration merging, but it might also be by design. (And honestly "by design" would be nice in my particular use case.) Here it is:

// ModuleA.h
void test() {
  extern void sharedFunc(void);
}

// ModuleB.h
void sharedFunc(int);

// client.c
#import <ModuleA.h>
#import <ModuleB.h>
// or the other order, for a slightly different experiment

In this situation, the locally-scoped declaration of 'sharedFunc' in ModuleA isn't resolved with the declaration in ModuleB. There's even a conflict in their types! Is this by design, or an oversight, or what?

Well.. we only merge declarations that have matching types. This decision predates my involvement, but I think it makes some sense: we probably don't want the deserialization code somewhat nondeterministically producing diagnostics when it notices problems like this, and deferring the diagnostic to an ambiguity error when the function is actually used seems like the most consistent user experience. And that ambiguity error will presumably not appear for a call to sharedFunc(), because (non-redeclaration) lookup for sharedFunc won't find the local extern declaration in 'test'. (But I would not be surprised to see an error if you attempt to redeclare sharedFunc after importing both modules, or if you try to build both headers as part of the same module.)

Regardless of whether we accept it, the above program is ill-formed (undefined in C / ill-formed with no diagnostic required in C++), so I would encourage you to limit your reliance on it. I don't think we want to guarantee that such code keeps compiling.

One of the consequences of this is that in client.c's context, asking for the canonical decl for 'sharedFunc' gives me the declaration in ModuleB. From client.c's perspective, that's more useful than giving me the one in ModuleA, but it's not consistent with the behavior with modules turned off.

Any insights to offer? Should I file this at bugs.llvm.org?

We generally don't produce eager diagnostics for conflicts between imported modules, and that's a feature: we want module import to be constant-time, which means we can't diagnose such things eagerly. So for the above program (with no use of sharedFunc), I don't think we should produce a diagnostic.

I think the current behavior is largely by design.

Thanks for the explanation. There's still weirdness here even if they do have the same types, since the canonical decl is going to be different, and that can have user-visible effects if, say, the local declaration of 'sharedFunc' has an attribute like warn_unused_result. Is that by design? (I made the types different to be provocative and to make the problem obvious.)

I think at least part of this is a more general issue unrelated to local extern declarations. Inheritable attributes are not inherited during merging, so if the same declaration appears in two different modules with different sets of accumulated attributes at the end of each module, you will get surprising behavior.

I know module loading does make an effort to merge redeclaration chains normally; it's only locally-scoped extern declarations that are being left out.

Is this issue C-specific? (I could easily believe that we don't properly query/update the global scope in this case, and so don't end up merging local extern declarations, but I think we do update the translation unit DeclContext -- and merge declarations such as the above -- in C++.)

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Modules, redeclaration chains, and locally-scoped extern declarations

Kristof Beyls via cfe-dev


On Sep 26, 2019, at 16:00, Richard Smith <[hidden email]> wrote:

On Thu, 26 Sep 2019 at 15:08, Jordan Rose via cfe-dev <[hidden email]> wrote:
On Sep 26, 2019, at 14:50, Richard Smith <[hidden email]> wrote:

On Thu, 26 Sep 2019 at 14:38, Jordan Rose via cfe-dev <[hidden email]> wrote:
Hi, modules folks and cfe-dev. I think I've discovered a bug in how module loading handles redeclaration merging, but it might also be by design. (And honestly "by design" would be nice in my particular use case.) Here it is:

// ModuleA.h
void test() {
  extern void sharedFunc(void);
}

// ModuleB.h
void sharedFunc(int);

// client.c
#import <ModuleA.h>
#import <ModuleB.h>
// or the other order, for a slightly different experiment

In this situation, the locally-scoped declaration of 'sharedFunc' in ModuleA isn't resolved with the declaration in ModuleB. There's even a conflict in their types! Is this by design, or an oversight, or what?

Well.. we only merge declarations that have matching types. This decision predates my involvement, but I think it makes some sense: we probably don't want the deserialization code somewhat nondeterministically producing diagnostics when it notices problems like this, and deferring the diagnostic to an ambiguity error when the function is actually used seems like the most consistent user experience. And that ambiguity error will presumably not appear for a call to sharedFunc(), because (non-redeclaration) lookup for sharedFunc won't find the local extern declaration in 'test'. (But I would not be surprised to see an error if you attempt to redeclare sharedFunc after importing both modules, or if you try to build both headers as part of the same module.)

Regardless of whether we accept it, the above program is ill-formed (undefined in C / ill-formed with no diagnostic required in C++), so I would encourage you to limit your reliance on it. I don't think we want to guarantee that such code keeps compiling.

One of the consequences of this is that in client.c's context, asking for the canonical decl for 'sharedFunc' gives me the declaration in ModuleB. From client.c's perspective, that's more useful than giving me the one in ModuleA, but it's not consistent with the behavior with modules turned off.

Any insights to offer? Should I file this at bugs.llvm.org?

We generally don't produce eager diagnostics for conflicts between imported modules, and that's a feature: we want module import to be constant-time, which means we can't diagnose such things eagerly. So for the above program (with no use of sharedFunc), I don't think we should produce a diagnostic.

I think the current behavior is largely by design.

Thanks for the explanation. There's still weirdness here even if they do have the same types, since the canonical decl is going to be different, and that can have user-visible effects if, say, the local declaration of 'sharedFunc' has an attribute like warn_unused_result. Is that by design? (I made the types different to be provocative and to make the problem obvious.)

I think at least part of this is a more general issue unrelated to local extern declarations. Inheritable attributes are not inherited during merging, so if the same declaration appears in two different modules with different sets of accumulated attributes at the end of each module, you will get surprising behavior.

Fair enough.


I know module loading does make an effort to merge redeclaration chains normally; it's only locally-scoped extern declarations that are being left out.

Is this issue C-specific? (I could easily believe that we don't properly query/update the global scope in this case, and so don't end up merging local extern declarations, but I think we do update the translation unit DeclContext -- and merge declarations such as the above -- in C++.)

It seems to have the same behavior in both C and C++. That is, in the case where the declarations are identical in both source files, walking redecls() only ever shows one entry (the one in the top-level module).

I admit I'm testing this using the Swift compiler rather than Clang itself, which does a funny thing where it imports a module as hidden and then again as visible. I'll check what happens if we don't do the hidden step, which is closer to the simple case for Clang.

Jordan


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Modules, redeclaration chains, and locally-scoped extern declarations

Kristof Beyls via cfe-dev


On Sep 27, 2019, at 15:36, Jordan Rose via cfe-dev <[hidden email]> wrote:



On Sep 26, 2019, at 16:00, Richard Smith <[hidden email]> wrote:

On Thu, 26 Sep 2019 at 15:08, Jordan Rose via cfe-dev <[hidden email]> wrote:
On Sep 26, 2019, at 14:50, Richard Smith <[hidden email]> wrote:

On Thu, 26 Sep 2019 at 14:38, Jordan Rose via cfe-dev <[hidden email]> wrote:
Hi, modules folks and cfe-dev. I think I've discovered a bug in how module loading handles redeclaration merging, but it might also be by design. (And honestly "by design" would be nice in my particular use case.) Here it is:

// ModuleA.h
void test() {
  extern void sharedFunc(void);
}

// ModuleB.h
void sharedFunc(int);

// client.c
#import <ModuleA.h>
#import <ModuleB.h>
// or the other order, for a slightly different experiment

In this situation, the locally-scoped declaration of 'sharedFunc' in ModuleA isn't resolved with the declaration in ModuleB. There's even a conflict in their types! Is this by design, or an oversight, or what?

Well.. we only merge declarations that have matching types. This decision predates my involvement, but I think it makes some sense: we probably don't want the deserialization code somewhat nondeterministically producing diagnostics when it notices problems like this, and deferring the diagnostic to an ambiguity error when the function is actually used seems like the most consistent user experience. And that ambiguity error will presumably not appear for a call to sharedFunc(), because (non-redeclaration) lookup for sharedFunc won't find the local extern declaration in 'test'. (But I would not be surprised to see an error if you attempt to redeclare sharedFunc after importing both modules, or if you try to build both headers as part of the same module.)

Regardless of whether we accept it, the above program is ill-formed (undefined in C / ill-formed with no diagnostic required in C++), so I would encourage you to limit your reliance on it. I don't think we want to guarantee that such code keeps compiling.

One of the consequences of this is that in client.c's context, asking for the canonical decl for 'sharedFunc' gives me the declaration in ModuleB. From client.c's perspective, that's more useful than giving me the one in ModuleA, but it's not consistent with the behavior with modules turned off.

Any insights to offer? Should I file this at bugs.llvm.org?

We generally don't produce eager diagnostics for conflicts between imported modules, and that's a feature: we want module import to be constant-time, which means we can't diagnose such things eagerly. So for the above program (with no use of sharedFunc), I don't think we should produce a diagnostic.

I think the current behavior is largely by design.

Thanks for the explanation. There's still weirdness here even if they do have the same types, since the canonical decl is going to be different, and that can have user-visible effects if, say, the local declaration of 'sharedFunc' has an attribute like warn_unused_result. Is that by design? (I made the types different to be provocative and to make the problem obvious.)

I think at least part of this is a more general issue unrelated to local extern declarations. Inheritable attributes are not inherited during merging, so if the same declaration appears in two different modules with different sets of accumulated attributes at the end of each module, you will get surprising behavior.

Fair enough.


I know module loading does make an effort to merge redeclaration chains normally; it's only locally-scoped extern declarations that are being left out.

Is this issue C-specific? (I could easily believe that we don't properly query/update the global scope in this case, and so don't end up merging local extern declarations, but I think we do update the translation unit DeclContext -- and merge declarations such as the above -- in C++.)

It seems to have the same behavior in both C and C++. That is, in the case where the declarations are identical in both source files, walking redecls() only ever shows one entry (the one in the top-level module).

I admit I'm testing this using the Swift compiler rather than Clang itself, which does a funny thing where it imports a module as hidden and then again as visible. I'll check what happens if we don't do the hidden step, which is closer to the simple case for Clang.

No difference. :-(


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Modules, redeclaration chains, and locally-scoped extern declarations

Kristof Beyls via cfe-dev
In reply to this post by Kristof Beyls via cfe-dev
On Fri, 27 Sep 2019 at 15:36, Jordan Rose via cfe-dev <[hidden email]> wrote:
On Sep 26, 2019, at 16:00, Richard Smith <[hidden email]> wrote:

On Thu, 26 Sep 2019 at 15:08, Jordan Rose via cfe-dev <[hidden email]> wrote:
On Sep 26, 2019, at 14:50, Richard Smith <[hidden email]> wrote:

On Thu, 26 Sep 2019 at 14:38, Jordan Rose via cfe-dev <[hidden email]> wrote:
Hi, modules folks and cfe-dev. I think I've discovered a bug in how module loading handles redeclaration merging, but it might also be by design. (And honestly "by design" would be nice in my particular use case.) Here it is:

// ModuleA.h
void test() {
  extern void sharedFunc(void);
}

// ModuleB.h
void sharedFunc(int);

// client.c
#import <ModuleA.h>
#import <ModuleB.h>
// or the other order, for a slightly different experiment

In this situation, the locally-scoped declaration of 'sharedFunc' in ModuleA isn't resolved with the declaration in ModuleB. There's even a conflict in their types! Is this by design, or an oversight, or what?

Well.. we only merge declarations that have matching types. This decision predates my involvement, but I think it makes some sense: we probably don't want the deserialization code somewhat nondeterministically producing diagnostics when it notices problems like this, and deferring the diagnostic to an ambiguity error when the function is actually used seems like the most consistent user experience. And that ambiguity error will presumably not appear for a call to sharedFunc(), because (non-redeclaration) lookup for sharedFunc won't find the local extern declaration in 'test'. (But I would not be surprised to see an error if you attempt to redeclare sharedFunc after importing both modules, or if you try to build both headers as part of the same module.)

Regardless of whether we accept it, the above program is ill-formed (undefined in C / ill-formed with no diagnostic required in C++), so I would encourage you to limit your reliance on it. I don't think we want to guarantee that such code keeps compiling.

One of the consequences of this is that in client.c's context, asking for the canonical decl for 'sharedFunc' gives me the declaration in ModuleB. From client.c's perspective, that's more useful than giving me the one in ModuleA, but it's not consistent with the behavior with modules turned off.

Any insights to offer? Should I file this at bugs.llvm.org?

We generally don't produce eager diagnostics for conflicts between imported modules, and that's a feature: we want module import to be constant-time, which means we can't diagnose such things eagerly. So for the above program (with no use of sharedFunc), I don't think we should produce a diagnostic.

I think the current behavior is largely by design.

Thanks for the explanation. There's still weirdness here even if they do have the same types, since the canonical decl is going to be different, and that can have user-visible effects if, say, the local declaration of 'sharedFunc' has an attribute like warn_unused_result. Is that by design? (I made the types different to be provocative and to make the problem obvious.)

I think at least part of this is a more general issue unrelated to local extern declarations. Inheritable attributes are not inherited during merging, so if the same declaration appears in two different modules with different sets of accumulated attributes at the end of each module, you will get surprising behavior.

Fair enough.


I know module loading does make an effort to merge redeclaration chains normally; it's only locally-scoped extern declarations that are being left out.

Is this issue C-specific? (I could easily believe that we don't properly query/update the global scope in this case, and so don't end up merging local extern declarations, but I think we do update the translation unit DeclContext -- and merge declarations such as the above -- in C++.)

It seems to have the same behavior in both C and C++. That is, in the case where the declarations are identical in both source files, walking redecls() only ever shows one entry (the one in the top-level module).

I only see the missing declaration in C. Testcase:

#pragma clang module build foo
module foo {}
#pragma clang module contents
#pragma clang module begin foo
void test() {
  extern void f();
}
#pragma clang module end
#pragma clang module endbuild

#pragma clang module build bar
module bar {}
#pragma clang module contents
#pragma clang module begin bar
void f();
#pragma clang module end
#pragma clang module endbuild

#pragma clang module import foo
#pragma clang module import bar

#pragma clang __debug dump f

In C++ mode, I see:

lookup results for f:
FunctionDecl 0xdd52960 prev 0xdd52888 </usr/local/google/home/richardsmith/clang-mono-1/bar.map:4:1, col:8> col:6 imported in bar f 'void ()'

In C mode, I see:

lookup results for f:
FunctionDecl 0xeda8018 </usr/local/google/home/richardsmith/clang-mono-1/bar.map:4:1, col:8> col:6 imported in bar f 'void ()'

(So: in C++ there's a previous declaration, and in C there isn't.)

If I add:

void g() { test(); }

then the definition of test() is imported and the declaration of the local extern function is properly merged into the redeclaration chain. (I mean, maybe that's mostly OK for the purpose of compiling: we generally don't need the local declaration of 'f' unless we need the definition of the enclosing function. But it is unfortunate that walking the redeclaration chain misses things; it's not supposed to, and I think some parts of Clang rely on the redeclaration chain not spontaneously changing after you start walking it, which could happen here. And in any case for uses of Clang as a library, walking the redeclaration chain really needs to give the right answer.)
 
I admit I'm testing this using the Swift compiler rather than Clang itself, which does a funny thing where it imports a module as hidden and then again as visible. I'll check what happens if we don't do the hidden step, which is closer to the simple case for Clang.

Jordan

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Modules, redeclaration chains, and locally-scoped extern declarations

Kristof Beyls via cfe-dev


On Sep 27, 2019, at 16:39, Richard Smith <[hidden email]> wrote:

On Fri, 27 Sep 2019 at 15:36, Jordan Rose via cfe-dev <[hidden email]> wrote:
On Sep 26, 2019, at 16:00, Richard Smith <[hidden email]> wrote:

On Thu, 26 Sep 2019 at 15:08, Jordan Rose via cfe-dev <[hidden email]> wrote:
On Sep 26, 2019, at 14:50, Richard Smith <[hidden email]> wrote:

On Thu, 26 Sep 2019 at 14:38, Jordan Rose via cfe-dev <[hidden email]> wrote:
Hi, modules folks and cfe-dev. I think I've discovered a bug in how module loading handles redeclaration merging, but it might also be by design. (And honestly "by design" would be nice in my particular use case.) Here it is:

// ModuleA.h
void test() {
  extern void sharedFunc(void);
}

// ModuleB.h
void sharedFunc(int);

// client.c
#import <ModuleA.h>
#import <ModuleB.h>
// or the other order, for a slightly different experiment

In this situation, the locally-scoped declaration of 'sharedFunc' in ModuleA isn't resolved with the declaration in ModuleB. There's even a conflict in their types! Is this by design, or an oversight, or what?

Well.. we only merge declarations that have matching types. This decision predates my involvement, but I think it makes some sense: we probably don't want the deserialization code somewhat nondeterministically producing diagnostics when it notices problems like this, and deferring the diagnostic to an ambiguity error when the function is actually used seems like the most consistent user experience. And that ambiguity error will presumably not appear for a call to sharedFunc(), because (non-redeclaration) lookup for sharedFunc won't find the local extern declaration in 'test'. (But I would not be surprised to see an error if you attempt to redeclare sharedFunc after importing both modules, or if you try to build both headers as part of the same module.)

Regardless of whether we accept it, the above program is ill-formed (undefined in C / ill-formed with no diagnostic required in C++), so I would encourage you to limit your reliance on it. I don't think we want to guarantee that such code keeps compiling.

One of the consequences of this is that in client.c's context, asking for the canonical decl for 'sharedFunc' gives me the declaration in ModuleB. From client.c's perspective, that's more useful than giving me the one in ModuleA, but it's not consistent with the behavior with modules turned off.

Any insights to offer? Should I file this at bugs.llvm.org?

We generally don't produce eager diagnostics for conflicts between imported modules, and that's a feature: we want module import to be constant-time, which means we can't diagnose such things eagerly. So for the above program (with no use of sharedFunc), I don't think we should produce a diagnostic.

I think the current behavior is largely by design.

Thanks for the explanation. There's still weirdness here even if they do have the same types, since the canonical decl is going to be different, and that can have user-visible effects if, say, the local declaration of 'sharedFunc' has an attribute like warn_unused_result. Is that by design? (I made the types different to be provocative and to make the problem obvious.)

I think at least part of this is a more general issue unrelated to local extern declarations. Inheritable attributes are not inherited during merging, so if the same declaration appears in two different modules with different sets of accumulated attributes at the end of each module, you will get surprising behavior.

Fair enough.


I know module loading does make an effort to merge redeclaration chains normally; it's only locally-scoped extern declarations that are being left out.

Is this issue C-specific? (I could easily believe that we don't properly query/update the global scope in this case, and so don't end up merging local extern declarations, but I think we do update the translation unit DeclContext -- and merge declarations such as the above -- in C++.)

It seems to have the same behavior in both C and C++. That is, in the case where the declarations are identical in both source files, walking redecls() only ever shows one entry (the one in the top-level module).

I only see the missing declaration in C. Testcase:

#pragma clang module build foo
module foo {}
#pragma clang module contents
#pragma clang module begin foo
void test() {
  extern void f();
}
#pragma clang module end
#pragma clang module endbuild

#pragma clang module build bar
module bar {}
#pragma clang module contents
#pragma clang module begin bar
void f();
#pragma clang module end
#pragma clang module endbuild

#pragma clang module import foo
#pragma clang module import bar

#pragma clang __debug dump f

In C++ mode, I see:

lookup results for f:
FunctionDecl 0xdd52960 prev 0xdd52888 </usr/local/google/home/richardsmith/clang-mono-1/bar.map:4:1, col:8> col:6 imported in bar f 'void ()'

In C mode, I see:

lookup results for f:
FunctionDecl 0xeda8018 </usr/local/google/home/richardsmith/clang-mono-1/bar.map:4:1, col:8> col:6 imported in bar f 'void ()'

(So: in C++ there's a previous declaration, and in C there isn't.)

If I add:

void g() { test(); }

then the definition of test() is imported and the declaration of the local extern function is properly merged into the redeclaration chain. (I mean, maybe that's mostly OK for the purpose of compiling: we generally don't need the local declaration of 'f' unless we need the definition of the enclosing function. But it is unfortunate that walking the redeclaration chain misses things; it's not supposed to, and I think some parts of Clang rely on the redeclaration chain not spontaneously changing after you start walking it, which could happen here. And in any case for uses of Clang as a library, walking the redeclaration chain really needs to give the right answer.)

In Apple Clang (the one in the upcoming Xcode 11) I get a "prev" in both modes, so now I'm totally lost. Which means for my particular use case I should probably just Not Depend On Any Particular Behavior. Not having an isolated test case to send to Clang makes me think it's not worth pursuing right now.

Thanks for all the help,
Jordan


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev