Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

error: declaration '__builtin_trap' attached to named module 'Invariant' can't be attached to other modules #3

Closed
chriselrod opened this issue Jul 9, 2024 · 6 comments

Comments

@chriselrod
Copy link

chriselrod commented Jul 9, 2024

/path/to/project/mod/Utilities/Invariant.cxx:18:5: error: declaration '__builtin_trap' attached to named module 'Invariant' can't be attached to other modules
   18 |     __builtin_trap();
      |     ^
/usr/lib/gcc/x86_64-redhat-linux/14/../../../../include/c++/14/array:74:49: note: also found
   74 |        _Tp& operator[](size_t) const noexcept { __builtin_trap(); }
      |                                                 ^
1 error generated.

This is a regression compared to clang 18.1.6 or llvm/llvm-project#66462.

My code calling __builtin_trap() no longer compiles with this branch of clang++.

I am also getting this for modules that instantiate class templates declared/defined in other modules (when this worked with older versions of clang++).

@ChuanqiXu9
Copy link
Owner

Can you provide a reproducer or at best a minimal reproducer?

@ChuanqiXu9
Copy link
Owner

BTW, the error message looks not like clangd's error message. If it is from the compiler, please register it in the upstream repo.

@chriselrod
Copy link
Author

Yes, that was from clang++ on this branch.
I can reproduce on a recent clang master, so I'll close this.

I'll open an issue on llvm-project if I'm able to get a minimal reproducer, but so far my attempts at a minimal reproducer failed.

@leha-bot
Copy link

Sorry, @chriselrod for ping, but did you create an issue in llvm-project? If yes, may you give an issue link please?

Thanks for your attention.

@chriselrod
Copy link
Author

chriselrod commented Jul 17, 2024

I failed to get a minimal reproducer, so I haven't created an issue yet.
I actually no longer saw this, but do see one about builtins such as __builtin_add_overflow.
A perhaps necessary but definitely insufficient condition is that the builtin must be used in at least two separate modules.

One of my dependencies was using these intrinsics, as well as some of my own code. By replacing all uses within my own code with explicitly typed versions (e.g. __builtin_saddl_overflow), I could work around the issue.

But then I got linker errors for some exported module functions. I made one attempt at a minimal reproducer for this, but it also worked fine.
I haven't found a workaround, and have instead went back to working header-only for the time being.
I may switch to header+source file to try and improve my incremental compile times.
The approach I took with modularizing didn't improve these; incremental with modules rebuilding leaves of the dependency tree were already as slow as full rebuilds of the test suite with header-only.
But I'll try and think/read more before making major changes to my code base that may not end up paying off.

Something I didn't take advantage but probably should have are implementation units, to hide implementations.
Ideally, I could use preprocessor macros and some cmakes to make the same source be header+source file vs module interface + module implementation files.
I also think that I want fewer translations units than the number of header files that I have.

import std; may improve things once I switch to cmake 3.30 (I'm currently on 3.28).
My header-only version uses precompiled headers, while I wasn't using them with modules.
Although, I also have headers from dependencies that include in the PCH, for which I'd have to pay the parsing cost with modules -- unless there is a way to use PCH + modules?

@ChuanqiXu9
Copy link
Owner

I failed to get a minimal reproducer, so I haven't created an issue yet. I actually no longer saw this, but do see one about builtins such as __builtin_add_overflow. A perhaps necessary but definitely insufficient condition is that the builtin must be used in at least two separate modules.

One of my dependencies was using these intrinsics, as well as some of my own code. By replacing all uses within my own code with explicitly typed versions (e.g. __builtin_saddl_overflow), I could work around the issue.

But then I got linker errors for some exported module functions. I made one attempt at a minimal reproducer for this, but it also worked fine. I haven't found a workaround, and have instead went back to working header-only for the time being. I may switch to header+source file to try and improve my incremental compile times. The approach I took with modularizing didn't improve these; incremental with modules rebuilding leaves of the dependency tree were already as slow as full rebuilds of the test suite with header-only. But I'll try and think/read more before making major changes to my code base that may not end up paying off.

Something I didn't take advantage but probably should have are implementation units, to hide implementations. Ideally, I could use preprocessor macros and some cmakes to make the same source be header+source file vs module interface + module implementation files. I also think that I want fewer translations units than the number of header files that I have.

import std; may improve things once I switch to cmake 3.30 (I'm currently on 3.28). My header-only version uses precompiled headers, while I wasn't using them with modules. Although, I also have headers from dependencies that include in the PCH, for which I'd have to pay the parsing cost with modules -- unless there is a way to use PCH + modules?

Yeah, it is suggested to use implementation units to hide implementation more. There is also an option called reduced BMI to help that (https://clang.llvm.org/docs/StandardCPlusPlusModules.html#reduced-bmi).

For rebuilding, there is a WIP in llvm/llvm-project#96453. But end users need the help from build systems to use that.

For PCH + modules, technically it might be fine. But there is not so people tested it. And also personally, I feel it may not be necessary to do that.

ChuanqiXu9 pushed a commit that referenced this issue Jul 18, 2024
This patch adds a frame recognizer for Clang's
`__builtin_verbose_trap`, which behaves like a
`__builtin_trap`, but emits a failure-reason string into debug-info in
order for debuggers to display
it to a user.

The frame recognizer triggers when we encounter
a frame with a function name that begins with
`__clang_trap_msg`, which is the magic prefix
Clang emits into debug-info for verbose traps.
Once such frame is encountered we display the
frame function name as the `Stop Reason` and display that frame to the
user.

Example output:
```
(lldb) run
warning: a.out was compiled with optimization - stepping may behave oddly; variables may not be available.
Process 35942 launched: 'a.out' (arm64)
Process 35942 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = Misc.: Function is not implemented
    frame #1: 0x0000000100003fa4 a.out`main [inlined] Dummy::func(this=<unavailable>) at verbose_trap.cpp:3:5 [opt]
   1    struct Dummy {
   2      void func() {
-> 3        __builtin_verbose_trap("Misc.", "Function is not implemented");
   4      }
   5    };
   6
   7    int main() {
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = Misc.: Function is not implemented
    frame #0: 0x0000000100003fa4 a.out`main [inlined] __clang_trap_msg$Misc.$Function is not implemented$ at verbose_trap.cpp:0 [opt]
  * frame #1: 0x0000000100003fa4 a.out`main [inlined] Dummy::func(this=<unavailable>) at verbose_trap.cpp:3:5 [opt]
    frame #2: 0x0000000100003fa4 a.out`main at verbose_trap.cpp:8:13 [opt]
    frame #3: 0x0000000189d518b4 dyld`start + 1988
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants