Details
Diff Detail
Event Timeline
I'm in C standards meetings this week and don't have a lot of ability to thoroughly review this yet, but the most immediate concern which springs to mind for me is that this is exposing LLVM implementation details to users. Users should not have to think about things in terms of LLVM IR markings like poison and undef, and I worry that this is an expert-only feature that will be easy to misuse and introduce security issues. It sounds like there's an extremely specific use case in mind for this attribute; is there a reason why this needs to be the user's problem instead of something the compiler can infer or handle on the backend?
The intended user is a handful of header wrapper functions around intrinsics, not general users
Great, then it sounds like we don't need to expose this as an attribute because we know which intrinsics need special behavior, so we can handle them specially at codegen time or within the backend?
The problem is the wrappers. We originally talked about specifically handling the builtins, but that falls apart given the real users come from wrapper functions which look like ordinary code
Here's how I would tentatively describe the attribute in terms that mesh better with how I understand C and C++:
As an exception to the rule that loading from an unitialized variable is undefined behavior, if the loaded value is used immediately as an __attribute__((maybe_undef)) argument in a function call, the loaded value is implementation-defined. It may vary between multiple runs of the program, and it may vary between multiple uses of the uninitialized variable.
This requires no thinking about LLVM IR and undef/poison.
There may have to be some caveats about bools and enums (generally, types where not all possible values in memory are actually legal values of the type), but I don't know enough about those language standards to judge that.
clang/include/clang/Basic/AttrDocs.td | ||
---|---|---|
276 | param shouldn't be of void type, right? |
Generating freeze seems an option, just not sure we can easily get all locations with this approach.
Anyway, some code comments below.
clang/lib/CodeGen/CGCall.cpp | ||
---|---|---|
2061 | Early exists please. if (!TargetDecl) return false no ArgHasMayBeUndefAttr is needed, just return true/false, etc. | |
4906 | Style: Val, also below. | |
clang/test/CodeGenHIP/maybe_undef-attr-verify.hip | ||
34 | Note, not necessarily to address: I would have preferred auto generated check lines, should not be many and it's easier to see what's going on, IMHO. |
I'm still not seeing the issue fully. My understanding of the situation (which may be wrong) is that Clang lowers to LLVM IR and adds noundef markings at call sites that this patch attempts to let the user then undo. However, why can Clang's CodeGen not notice the special builtin and walk up the call chain to mark all the wrapper functions to ensure everything is marked appropriately? There might be a small perf loss (if there are wrappers around wrappers around wrappers kind of situation), but this means we don't have to expose this problem to users.
Thanks, this helps somewhat (though I would argue that the loaded value is *not* implementation-defined but is instead unspecified, because that matches the semantics you mention about not needing to pick the same loaded value every time). However, this design is basically asking the user to decide when something should or should not be undefined behavior, which means this attribute will almost certainly be abused under the guise of "this makes my program safer because it removes UB". I still think that a solution which doesn't require the user to make such dangerous decisions on their own is a better design if we can accomplish it.
This requires maintaining logic in clang to specially recognize these N wrapper functions, and there would be no indication in the source that this magic is occurring. It's less maintainable because it requires tight, magic coupling between these functions in the source and special handling in clang to be maintained if anything were to change
I'm still not seeing the issue fully. My understanding of the situation (which may be wrong) is that Clang lowers to LLVM IR and adds noundef markings at call sites that this patch attempts to let the user then undo. However, why can Clang's CodeGen not notice the special builtin and walk up the call chain to mark all the wrapper functions to ensure everything is marked appropriately? There might be a small perf loss (if there are wrappers around wrappers around wrappers kind of situation), but this means we don't have to expose this problem to users.
I think you are right, we could identify shuffles and go up the call chain in clang (codegen) if we wanted.
I still think that a solution which doesn't require the user to make such dangerous decisions on their own is a better design if we can accomplish it.
This is a valid opinion but I don't see how this matches what we do (or has been done) in the past. I mean, there are plenty of command line options and attributes that do exactly this, let the user "define", or better "refine", semantics.
The entire "initialize with 0" stuff comes to mind, making new things UB with -ffast-math and friends, overloadable, etc.
All that said, I'm generally more in favor of explicit annotations in the source than implicit compiler magic to implement a special case.
Others should chime in.
I was thinking of something more general than that where Clang only has to specially recognize the builtins and which arguments to the builtin should be tracked to make maybeundef when walking up the call chain. However, you're not wrong about this being a bit magical in terms of behavior. I don't know that we have anything else which behaves this way.
Thanks, this is a good question to be asking!
We are adding attributes in a way which doesn't scale well; many attributes interact with one another and we have basically no test coverage for those situations and very little thought put into diagnostic behavior. Now that we've got over 400 attributes in the tree, I'm realizing I probably should have been saying "no" more to attributes that don't carry enough weight to be worth the maintenance burden, but oh well, I can't unring that bell. I have *no issue* with attributes that are going to be things we expect users to use themselves. Stuff like "initialize with 0", overloadable, etc are all things that enable users to do something they couldn't previously do. However, in recent history, we've been adding more attributes that are for compiler glue code rather than users. On the one hand, attributes are a great use for such a thing. On the other hand, there's no way to control who uses the attribute (unless we put in special logic to only allow it to be written in a system header, or something like that). So glue code attributes are a different "thing" to me than user-facing attributes, and I'm worried at how many we're adding without any real design thought put behind them other than "we need it to solve this one case". If we could come up with some tablegen marking to say "this attribute is for glue code use only" that we could use to generate diagnostics on "misuse", generate documentation differently from, etc, that would go a long ways towards alleviating the concerns about user confusion with glue code attributes and "real" attributes (for lack of a better term).
Of course, these are high-level worries and not necessarily specific to this review or anything I expect someone here to take action on. Take this as background information rather than a code owner mandate or anything like that. Specific to this review, my concern is that this is an attribute that controls undefined behavior in LLVM rather directly (as opposed to things like "initialize to 0" which impact UB, but aren't as tightly tied to the LLVM backend architecture). That's a worry for two reasons: the tight coupling to LLVM IR attribute semantics (basically, this means LLVM's "freeze" instruction can't change meaning because it will potentially break user code) and misuse due to misunderstanding the purpose of the attribute ("this makes my code more secure!"). Special compiler magic alleviates both of those concerns because it means we aren't exposing the tight coupling and users aren't given a chance to misuse the attribute.
However, what I think I'm hearing from this thread is that there are alternative approaches that have been thought about but not tried, we're not certain how feasible those approaches are in practice, but we expect them to be materially worse than what's proposed here. So it's not "this was the path of least resistance", but "this is the correct design." Do others agree with that assessment?
I think this is just the least bad on the menu of available options. I don't like it, but it at least provides documentation about this special behavior.
I think the only other plausible option is to assert this is still undefined behavior and force users to update their (newly declared invalid) code. We could at least partially re-optimize to uninitialized values in the backend (although this is apparently difficult in some situations)
First off, thank you for the great discussion about this both on and off lists while I wrapped my head around the design space and challenges involved. I appreciate your willingness to explore alternatives with me!
I think I agree that this is the least bad option on the menu. The code the user wrote is undefined behavior, and the CUDA/HIP standards would like to pretend otherwise in some circumstances. Given that users will have code invalidated if we stick to the hard line language rules, I think it's better than we remove the UB than punish the user and hope they can write a more convoluted but correct form.
In terms of the patch itself, I've left some comments. But this is also missing all semantic tests (that the attribute is diagnosed when written on something other than a parameter, accept no arguments, etc). It also is missing tests for some interesting use cases:
void func(int param); void other() { int derp; func(derp); // How should the call behave? } void func(__attribute__((maybe_undef)) int param) { ... } // Also: void foo(__attribute__((maybe_undef)) int param); void foo(int param) { ... } // Verify this parameter behaves as expected void bar() { int derp; foo(derp); }
It'd probably make sense to have a case involving templates as well to ensure that instantiation properly retains the parameter attribute, but we might already have test coverage for that (parameter attributes are pretty uncommon, but not unheard of).
clang/include/clang/Basic/Attr.td | ||
---|---|---|
2026 | ||
clang/include/clang/Basic/AttrDocs.td | ||
260 | ||
261 | I think this should be DocCatVariable temporarily, but we probably should add a new DocCatParameter category at some point and use that instead (in case you're feeling ambitious). | |
263 | ||
265 | ||
clang/lib/CodeGen/CGCall.cpp | ||
2050–2066 | One question I have is whether you ever need to mark the variadic arguments as being maybe undef. e.g., void func(int i, ...); do you need to signal that arguments passed to ... are maybe undef? |
clang/lib/CodeGen/CGCall.cpp | ||
---|---|---|
2050–2066 | Current change assumes variadic arguments will not have "maybe_undef" attribute. If its a function attribute, variadic arguments can inherit them (Have seen such cases in clang codebase). But "maybe_undef" is function argument attribute and I'm not sure on how to add it to variadic arguments. |
clang/lib/CodeGen/CGCall.cpp | ||
---|---|---|
2050–2066 |
That's why I was asking if there will be a need either now or in the future -- is making this a parameter attribute the right design, or should this be a function attribute with positional arguments? (Not that I particularly love that design, but this isn't a user-facing attribute so if it's hard to use, I'm not that worried. I just want to make sure we don't need to add additional attributes in the future in this same space.) |
I can't see a scenario where we would need this with a variadic call. Ultimately these wrap specific physical instructions, not some kind of arbitrary call
FWIW, precommit CI has some relevant failures for a change:
******************** TEST 'Clang :: CodeGen/attr-maybeundef-template.cpp' FAILED ******************** Script: -- : 'RUN: at line 1'; c:\ws\w4\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w4\llvm-project\premerge-checks\build\lib\clang\16.0.0\include -nostdsysteminc -no-opaque-pointers -emit-llvm C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef-template.cpp -o - | c:\ws\w4\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef-template.cpp -- Exit Code: 1 Command Output (stdout): -- $ ":" "RUN: at line 1" $ "c:\ws\w4\llvm-project\premerge-checks\build\bin\clang.exe" "-cc1" "-internal-isystem" "c:\ws\w4\llvm-project\premerge-checks\build\lib\clang\16.0.0\include" "-nostdsysteminc" "-no-opaque-pointers" "-emit-llvm" "C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef-template.cpp" "-o" "-" $ "c:\ws\w4\llvm-project\premerge-checks\build\bin\filecheck.exe" "C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef-template.cpp" # command stderr: C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef-template.cpp:3:11: error: CHECK: expected string not found in input // CHECK: define weak_odr void @_Z5test4IfEvT_(float noundef [[TMP1:%.*]]) ^ <stdin>:1:1: note: scanning from here ; ModuleID = 'C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef-template.cpp' ^ <stdin>:11:14: note: possible intended match here define weak_odr dso_local void @"??$test4@M@@YAXM@Z"(float noundef %arg) #0 comdat { ^ Input file: <stdin> Check file: C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef-template.cpp -dump-input=help explains the following input dump. Input was: <<<<<< 1: ; ModuleID = 'C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef-template.cpp' check:3'0 X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found 2: source_filename = "C:\\ws\\w4\\llvm-project\\premerge-checks\\clang\\test\\CodeGen\\attr-maybeundef-template.cpp" check:3'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 3: target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128" check:3'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 4: target triple = "x86_64-pc-windows-msvc" check:3'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 5: check:3'0 ~ 6: $"??$test4@M@@YAXM@Z" = comdat any check:3'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 7: check:3'0 ~ 8: $"??$test4@H@@YAXH@Z" = comdat any check:3'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 9: check:3'0 ~ 10: ; Function Attrs: mustprogress noinline nounwind optnone check:3'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 11: define weak_odr dso_local void @"??$test4@M@@YAXM@Z"(float noundef %arg) #0 comdat { check:3'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ check:3'1 ? possible intended match 12: entry: check:3'0 ~~~~~~~ 13: %arg.addr = alloca float, align 4 check:3'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 14: store float %arg, float* %arg.addr, align 4 check:3'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 15: ret void check:3'0 ~~~~~~~~~~ 16: } check:3'0 ~~ . . . >>>>>> error: command failed with exit status: 1 -- ******************** FAIL: Clang :: CodeGen/attr-maybeundef.c (4048 of 15750) ******************** TEST 'Clang :: CodeGen/attr-maybeundef.c' FAILED ******************** Script: -- : 'RUN: at line 1'; c:\ws\w4\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w4\llvm-project\premerge-checks\build\lib\clang\16.0.0\include -nostdsysteminc -no-opaque-pointers -emit-llvm C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef.c -o - | c:\ws\w4\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef.c -- Exit Code: 1 Command Output (stdout): -- $ ":" "RUN: at line 1" $ "c:\ws\w4\llvm-project\premerge-checks\build\bin\clang.exe" "-cc1" "-internal-isystem" "c:\ws\w4\llvm-project\premerge-checks\build\lib\clang\16.0.0\include" "-nostdsysteminc" "-no-opaque-pointers" "-emit-llvm" "C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef.c" "-o" "-" # command stderr: C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef.c:33:6: warning: 'maybe_undef' attribute only applies to parameters [-Wignored-attributes] void __maybe_undef t2(int param1, int param2, float param3) { ^ C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef.c:3:38: note: expanded from macro '__maybe_undef' #define __maybe_undef __attribute__((maybe_undef)) ^ 1 warning generated. $ "c:\ws\w4\llvm-project\premerge-checks\build\bin\filecheck.exe" "C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef.c" # command stderr: C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef.c:7:16: error: CHECK-NEXT: is not on the line after the previous match // CHECK-NEXT: [[TMP4:%.*]] = alloca i32, align 4 ^ <stdin>:10:2: note: 'next' match was here %param2.addr = alloca i32, align 4 ^ <stdin>:8:7: note: previous match ended here entry: ^ <stdin>:9:1: note: non-matching line after previous match is here %param3.addr = alloca float, align 4 ^ Input file: <stdin> Check file: C:\ws\w4\llvm-project\premerge-checks\clang\test\CodeGen\attr-maybeundef.c -dump-input=help explains the following input dump. Input was: <<<<<< . . . 5: 6: ; Function Attrs: noinline nounwind optnone 7: define dso_local void @t1(i32 noundef %param1, i32 noundef %param2, float noundef %param3) #0 { 8: entry: 9: %param3.addr = alloca float, align 4 10: %param2.addr = alloca i32, align 4 next:7 !~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: match on wrong line 11: %param1.addr = alloca i32, align 4 12: store float %param3, float* %param3.addr, align 4 13: store i32 %param2, i32* %param2.addr, align 4 14: store i32 %param1, i32* %param1.addr, align 4 15: ret void . . . >>>>>> error: command failed with exit status: 1 --
Excellent, that removes that concern. Thanks! Aside from the test failures and a minor coding style nit, I think this is close to ready to go.
clang/lib/CodeGen/CGCall.cpp | ||
---|---|---|
2063–2065 |
clang/test/CodeGen/attr-maybeundef-template.cpp | ||
---|---|---|
4 | This mangling works for Itanium targets, but doesn't match MSVC targets, which is why the test is failing on Windows. | |
clang/test/CodeGen/attr-maybeundef.c | ||
49 | It looks like dso_local is missing here on Windows and expected to be missing on Linux? Not certain what's up with that... |
clang/test/CodeGen/attr-maybeundef.c | ||
---|---|---|
49 | On Linux, dso_local isn't present. |
Would it be possible to implement this by dropping the noundef attribute rather than emitting a freeze? Seems like a much better option to me.
Thank you for the reference!
Seems like a big hammer, but I don't know the semantics of the GPU languages to comment further.
Hi, the test cases that this patch introduces are failing on some ppc64le (Linux on Power) buildbots:
https://lab.llvm.org/buildbot/#/builders/57
https://lab.llvm.org/buildbot/#/builders/230
Would you mind taking a look please?
Thanks for pinging this patch. It appears it's affecting both little endian PPC:
https://lab.llvm.org/buildbot/#/builders/230/builds/1079
https://lab.llvm.org/buildbot/#/builders/121/builds/21978
https://lab.llvm.org/buildbot/#/builders/57/builds/20484
https://lab.llvm.org/buildbot/#/builders/36/builds/23702
And big endian PPC bots:
https://lab.llvm.org/buildbot/#/builders/231/builds/842
https://lab.llvm.org/buildbot/#/builders/93/builds/10270
If we don't hear from @skc7 in the next ~hour with a fix, feel free to revert to get the bots back to green.
Issue is with missing target triple in the tests. Submitted D130790 for review, which should fix the tests.
Thanks @aaron.ballman. I've reverted the patch for the time being: https://reviews.llvm.org/rG4e1fe968c9de73507a1bf0c8aa57e06be457816e
I realized I didn't happen to see this comment in time and had already reverted the patch. My apologies on this.