[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

Michael Jabbour via cfe-commits cfe-commits at lists.llvm.org
Tue Jan 21 00:58:29 PST 2025


================
@@ -2551,18 +2551,7 @@ void Sema::MergeTypedefNameDecl(Scope *S, TypedefNameDecl *New,
       // Make the old tag definition visible.
       makeMergedDefinitionVisible(Hidden);
 
-      // If this was an unscoped enumeration, yank all of its enumerators
-      // out of the scope.
-      if (isa<EnumDecl>(NewTag)) {
-        Scope *EnumScope = getNonFieldDeclScope(S);
-        for (auto *D : NewTag->decls()) {
-          auto *ED = cast<EnumConstantDecl>(D);
-          assert(EnumScope->isDeclScope(ED));
-          EnumScope->RemoveDecl(ED);
-          IdResolver.RemoveDecl(ED);
-          ED->getLexicalDeclContext()->removeDecl(ED);
----------------
michael-jabbour-sonarsource wrote:

Thanks for raising the concern. I am also not entirely sure, but I chose to remove it for two reasons:

 1. It seems to me that the presence of this `removeDecl()` call causes an incorrect ODR violation diagnostic when `-ast-dump-all` is provided (which I am using in my regression test to expect the AST nodes after merging):

    ```
    In module 'ModB' imported from <source>:1:
    In module 'ModA' imported from ./ModBFile.h:2:
    ./shared.h:6:9: error: 'MyEnum3' has different definitions in different modules; definition in module 
    'ModA.ModAFile1' first difference is enum with 1 element
        6 | typedef enum { MyVal_C } MyEnum3;
          |         ^~~~~~~~~~~~~~~~
    ./shared.h:6:9: note: but in 'ModB' found enum with 0 elements
        6 | typedef enum { MyVal_C } MyEnum3;
          |         ^~~~~~~~~~~~~~~~
    1 error generated.
    ```

    This incorrect ODR violation can be observed before this patch. CE link: [here](https://godbolt.org/#z:OYLghAFBqd5QCxAFwE4FN0BoCWIIDGAtgIYDW6AgqsAM4gDkAtACIDCAspQNICiA%2BgCEAqgEkAMi34AVAJoAFXgFIAzCxboARgFdgDLAQD2RAA44ANulTiSAO2DaSwdKIAmIAkoBMg74IPatMjGAPLayCbhAGIW6LYkROgo6EFYAGax9LDorjjBqG4gXumxhSollvGJILQIJBiuAHQIWKAM%2Bji0orYE5tquOYwADB20HCQ4tgDKhtqoBEkMAIyjISZxw/rmdsCF7QCUWBA5eYYF7uUZloUALBVxCUnoAB4JJpaNBCYmrYyj3b1%2BoMGCNcGMJtNZvNFqDOmsNiCtjs9gxDsdcvlCkt7oUAGz3KpJIiGVzaD7E1ykH5tf49PoDdyIsHjSYzOYLRgrMHw2ybLDbeyFTw%2BPxok6Y9x3K4udwAdgJjxAHBJlBiliWzV%2B7TBAPpwNh4NZUI5y1W615TIFu3cwt8PjFGLOhQArDj3AAOBXVZWuVWxLyamk6ulAxkGlmQ9mLLlw818q1Cvyio7ip3ufHSwoATi9SR9gjV6EDf2DgIZfM6EbZ0M5ZoRoITNqT9sOnU45HQ8lQhgAVugCMha1geyBsbZ0AB3QtYoaHcyMZ36C2gwyMaQYdAAam8Ki5g8RaKQRgGvEdqFw%2BGlhJqxqLCG%2B6UMtmQUwIJEso6lT%2BQwloneefIjsUVoeM2/gmHY6AAHKKrU9Q5Jqf6WAOOBPvgcSuGwhh9EQvIqHcGHiJM0HaEQmhWCAromIYtB5KhthYTheF3NRtHIPRRHjlBpHkaglFYEh/bsU%2BUzIPUyCMaRzECegyHCdMYmoMgnEkWRFGukE4mSbhID4QJinKcR3FqXxzqHLQt6MAA9FZm7CD0xiJM%2BkzAJuyAIFu47PMgbkToYm4vG8li0G5qA4MAzioCF7lbgQqAkLUm4ZM8OSbpMbkIJ0m7yAASkoQyUDZbkAJ7rAMaQBbYpGbhwxW8FVRBLNusqCDVxUAGrvvwlDNSwbX1aRSyqL4BVFXE1W1QNRBeM1rW1Z15hCL1w35ZQq3IKVOToBV41ELNbULfwbC9f1DUqCtBWrbt%2B3zV1fVKLKLAXZQqL8guS58qucENJq%2B4jIeHgkugp6nOeeAQFeiqkJMnwPmk36vu%2BSRLF%2Bz6/v%2BgG6fyOygSKPhYBBXGKoFpgfF8PyCShaHHLYmHYVJo5YIRRk8RRLE0XRT7aby2KsZztgqcZvEgHclPyaJWn0zp2Ji/REtKYLrN8aLBnc4zmkKyzJki%2BZlkMDukxllu3hePmhbNCbq2rZMPnQ7YED7PtGDIHMtgHV1x1%2BO7i1PSorUPU9BWvfODCLlgy76KuJPvEW5NYH9r0QEeQMg/kF4Q7E16mo%2Bz6Ix%2BKM5z%2Bf7yABTIjtiIG2n4BOQTB1QUmSRYUlSMlyfR6G02r2LM1xSujvifPyV3BMc/Jiva0s%2BKyyJqtSzzrdCXLBnj8Lk/6ZLTHq8vWur7iutRowDeWDVKqzatm6bkfW4%2Bn66pnwVF8Xx5JADKg25eKbKqFhqCCWw/j8vGokpTcAAqc%2By1/5XxPr6QsM0HojR6o/Z%2Br936fxgf6C2H9wEX0AWcHyYD/4Byto9K2BUoH5nvog5BVhUFm1iJgrw4DcHAIIWtEhQc5zvXDp9EAV9GjNxID8BOANjzAzPOnSG1Rs7w1zm%2BfOqMi4YzLiLbG9hcZ2nArXRUN9v6IVkovamGFh491UsLYog96LDwsU%2BFeFFijTwUhvBmMt9FU0cZrXu2t7Gz03jLbenizH7xrPrLwKhDZ6lQd9BCv8sEcLeqHD6TJVw6NiD/eOmwREp3EeDSR0Z9AyJfHI5GCj0Yl0xq6SuYEa5E29F/DBLQHEdzpr4pmtNbF8XMaPSxc9GbWIFjvOxC83Hywkj0lxbcZ7iXaUUdeSlh4a0MgEuxQSTRFV4KYDaSVYirWDlwiOWBkl1MsAGFowijjJxPNky8mdFTSIRkUz8hdSml1BCOfElS8aaJqXmEkBZ6ENNcfJJpxi2kDNMiPNi3SWl9OmRpQFS8nHSyGeLfxpj1KzNGS0hZsKVmLCKnQ4%2BAxzSuBCk%2BaBt90AnM3E%2BcwxVGhW1CeEoEtCjmUoYatIqup%2BguU3FEpoCBNweQwJuN8gQUibimOgUgbl/KJBoFuGKm5KBTGkJuWwQMQppG7HtPlzRNwTjyAKxVmVXADDdk%2BcVWrjDkvpZdRlIYBiRLqD9GJjC4khzDvsw5rg/kfFORkw41C%2BIOywGQSiQx4k3H0EQUcQwI1epLPQbEZy4CwBgIgQGphYhg3wEYLNlg%2BK9B2PwBKf4lL0VoPwNA2hbBkAqMgKw9AICaD5JoSY9RipDjbfEVAxUQiaD7AOIceanLIBCLYWlfIBg6GAGwd85h6DZwGKQewH4mS5AwChAAbikPkLx%2BzhDybgZ8sl4w4E0PFXtWEBh8jQDgGN2cd2oE0DRdAGgV3AHMC5DJ6Rth0HajgScPIhwFLzsUp5xcXn6HLqo0AVd8Zfs0FkENhgTDyUXUwZ41KB2Lx3UwAgm4mBpCviFIjV8mBUiI9stQfCBEmEI2kWgxVnwkGeEwGlxVCMAA0i32EIwlZATBSSmCYPOyOT6woDGQ%2BZeFhjO5jNaa4aZvMulcwUzCsF6tZPuMxc45FCKPFor4n4xF89sWaaWIcNAmBRycISdwpJjBNzPHdLiJguIbgiqtJuCAparDodCjWsgjsIDrkwO/Xc%2Bx9DCMDegF%2BFEQ0hyjVgGNKh3SNFCbKXEsoliylc86d0QwbgqFdAm7U9AI0pvTUnEAswIjhHTnmswBa66LBc25jzXmdibiWFmRoGpQSgYeQXApzzMbJo3Dgdw2IJzxRMLWOznqeEMDCPVnyhgKrtfc553jrlfO0DLQF6ttaQtYXzTQnclnosBsjdGkAuINRZdCV4d0Xglj4V686fEZX9AVfSQeQ4YbnQRpDioRJK4SwxfiV4cHkdIc3afbRNCNwgA).

2. I am also not entirely sure if performing the call in the named enum case is safe. I can understand why it is important to remove the constant from IdResolver and the Enum context, but I don't know if removing the constant from the enum decl doesn't cause problems in some cases.

https://github.com/llvm/llvm-project/pull/114240


More information about the cfe-commits mailing list