https://github.com/dongjianqiang2 commented:
## Review: [Clang] Place constructor/destructor functions in
.text.startup/.text.exit
Thanks for this PR. The goal — matching GCC's behavior of placing
constructor/destructor functions in dedicated sections — makes sense and the
implementation is clean.
### Summary of changes
- Added `getCodeRegionSectionPrefix()` helper that returns `"startup"` for
`__attribute__((constructor))` and `"exit"` for `__attribute__((destructor))`.
- In `EmitGlobalFunctionDefinition`, calls `setSectionPrefix()` when the
function has no explicit section already set.
- Test covers IR metadata emission, assembly section emission with
`-ffunction-sections`, and the explicit-section escape hatch.
### What looks good
- **Correct mechanism**: Using `!section_prefix` IR metadata is the right
approach — the ELF backend already knows how to consume it to produce
`.text.startup` (without `-ffunction-sections`) or `.text.startup.<name>` (with
`-ffunction-sections`). This matches GCC's output exactly.
- **Good edge-case handling**: The `!Fn->hasSection()` guard correctly
preserves user-specified sections (e.g.,
`__attribute__((section(".my_section")))`), and the test verifies this with the
`CHECK-EXPLICIT-NOT` pattern.
- **Test coverage**: Covers plain constructor/destructor, priority variants,
assembly output, and the explicit-section escape hatch.
### Suggestions / Questions
1. **AI tool usage disclosure**: The commit has `Co-Authored-By:
deepseek-v4-pro`, but the PR description doesn't mention AI tool usage. Per the
LLVM AI Tool Use Policy, AI tool usage should be noted in the PR description.
Please add a note if AI tools were used.
2. **Target-agnostic**: The code sets section_prefix unconditionally (no triple
check). This is fine in practice because only ELF and WebAssembly backends
consume `!section_prefix` metadata — other targets ignore it. However,
WebAssembly doesn't have the `.text.startup` convention. If you want to be
precise, you could gate this behind an ELF check:
```cpp
if (getTarget().getTriple().isOSBinFormatELF())
```
But this is a minor point — the metadata is harmless on other targets.
3. **Test only covers aarch64-linux-gnu**: Consider adding an x86_64-linux-gnu
test case for broader coverage, though the mechanism is target-independent at
the IR level.
4. **C++ mode?**: The test only uses C (`clang_cc1` without `-x c++`). Since
`constructor`/`destructor` attributes work identically in C++ mode, this is
fine, but you might consider whether there are C++-specific constructor
patterns (e.g., `_GLOBAL__sub_I_*` synthesized by the compiler for global
initializers) that should also be treated. That's likely out of scope for this
PR though.
### Verdict
This is a well-scoped, clean change. The implementation is correct and matches
GCC behavior. My main ask is to add the AI tool usage note to the PR
description if applicable. Otherwise LGTM.
https://github.com/llvm/llvm-project/pull/200221
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
