Ah - this is an alternative to D104082? OK. Yeah, looks like it'd address the thing we were discussing in D98799. But do you have some more details on why D104082 wasn't the right direction? I thought we found other places that used a default-constructed/null GlobalDecl() for StartFunction` correctly, so I'm wondering why this would be a problem in this particular case(s)?

In D104777#2838211, @aaron.ballman wrote:

In D104777#2837794, @dblaikie wrote:

In D104777#2837347, @brunodefraine wrote:

In D104777#2836669, @dblaikie wrote:

Yeah, all that sounds reasonable to me - @brunodefraine could you look into supporting nodebug in a similar way as @aaron.ballman has described here?

Since the debuginfo for use() is slightly affected by the nodebug version of t1() that follows it, I can see how this back propagation is perhaps dangerous. Checking that nodebug is the same on all declarations of a function is a way to prevent this.

But when discussing the PR, @probinson wrote "I'm inclined to think we want this to work" and I can see what he means from the use case where I observed the bug. If you don't want debuginfo for the implementation of t1(), it should be fine to annotate just the function definition in an implementation file, not the declaration in a header, since the debuginfo of the implementation is not of the caller's concern. But nodebug as it exists does affect the debuginfo of callers as well, so I cannot really express that I don't want debuginfo for the implementation of a function and leave its callers unaffected?

I can see the convenience there, to be sure, being able to put the attribute directly on the function you want to debug - but consistency in how attributes are handled (admitedly this isn't a strong consistency - some are handled this way, some aren't) & consistently seeing the same state for an attribute for a given function seems useful.

@probinson - thoughts?

To me, this is the key bit:

But nodebug as it exists does affect the debuginfo of callers as well, so I cannot really express that I don't want debuginfo for the implementation of a function and leave its callers unaffected?

because it does affect the callers, the programmer introducing the API should be aware of that. Making this case an error helps them to understand that this attribute is actually a part of their API and not an implementation detail, and silently applying the attribute may cause hard-to-debug problems for them after deployment of their API.

In D99981#2837256, @tmiasko wrote:

For the record, support for Rust mangling scheme landed in:

• D101444
• D101821
• D101823
• D102142
• D102179
• D102518
• D102524
• D102549
• D102571
• D102573
• D102578
• D102579
• D102580
• D102581
• D102729
• D103151
• D103361
• D103364
• D103459
• D103460
• D103847
• D103848
• D104340
• D104358

With the only remaining part being a support for non-ASCII identifiers in D104366.

Thanks for review David!

In D104777#2837347, @brunodefraine wrote:

In D104777#2836669, @dblaikie wrote:

Yeah, all that sounds reasonable to me - @brunodefraine could you look into supporting nodebug in a similar way as @aaron.ballman has described here?

Since the debuginfo for use() is slightly affected by the nodebug version of t1() that follows it, I can see how this back propagation is perhaps dangerous. Checking that nodebug is the same on all declarations of a function is a way to prevent this.

But when discussing the PR, @probinson wrote "I'm inclined to think we want this to work" and I can see what he means from the use case where I observed the bug. If you don't want debuginfo for the implementation of t1(), it should be fine to annotate just the function definition in an implementation file, not the declaration in a header, since the debuginfo of the implementation is not of the caller's concern. But nodebug as it exists does affect the debuginfo of callers as well, so I cannot really express that I don't want debuginfo for the implementation of a function and leave its callers unaffected?

Do you have any interest in seeing if gdb could/would be fixed to handle the imported declaration style? Might be worth knowing if that's feasible, if they have some ideas about if/why that would be a bad idea, etc.

Sounds alright

In D104777#2836327, @aaron.ballman wrote:

In D104777#2835965, @dblaikie wrote:

@aaron.ballman Do we have attributes/infrastructure for attributes that have to be the same from their first declaration or at least from their first call? I'm wondering if it might be simpler/better to require nodebug to be provided earlier, rather than fixing it up after the fact like this.

(for instance, requiring it to be attributed on the declaration would ensure we don't create call_site descriptions for calls to nodebug functions - which would save some debug info size)

We don't have any attributes that have to be the same "from their first call" that I'm aware of. But we do have attributes that need to be the same on all declarations. IIRC, the section and code_seg attributes both have to be specified on the initial declaration in some circumstances, and [[noreturn]] needs to be on the first declaration we see. We don't have any tablegen machinery to enforce this, the logic needs to be manually written. I think it'd live somewhere around Sema::mergeDeclAttributes() or Sema::MergeFunctionDecl() most likely.

In D104342#2836222, @aaron.ballman wrote:

In D104342#2834240, @dblaikie wrote:

In D104342#2834119, @nickdesaulniers wrote:

I would think https://clang.llvm.org/docs/DiagnosticsReference.html#wframe-larger-than would be an appropriate place to document this for -Wframe-larger-than=, but it seems this whole page is generated via TableGen. It's not clear to me how we could insert such a note.

Yeah, I don't think we have a way to add more verbose/custom documentation for diagnostics. (@aaron.ballman might have some ideas)

I'm not aware of any way to do that today -- it would require more machinery for generating the diagnostic documentation, which is made harder by this particular diagnostic being a backend one that's not written out from tablegen.

In D103131#2835981, @kamleshbhalui wrote:

In D103131#2835909, @dblaikie wrote:

In D103131#2835702, @kamleshbhalui wrote:

Here is what we get when we replace int with float.

$lldb-11 ./a.out 
(lldb) target create "./a.out"
Current executable set to '/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin/a.out' (x86_64).
(lldb) b main
Breakpoint 1: where = a.out`main + 4 at test.c:3:12, address = 0x0000000000400484
(lldb) p oldname
(float) $0 = 1
(lldb) p newname
(void *) $1 = 0x000000003f800000

Yep, looks like it's ignoring the imported declaration in favor of the raw symbol table entry.

How's lldb handle GCC-style debug info (the variable declaration, without any imported declaration) for the alias?

lldb's behavior is same even with GCC -style debug info.

Sounds good, thanks!

@aaron.ballman Do we have attributes/infrastructure for attributes that have to be the same from their first declaration or at least from their first call? I'm wondering if it might be simpler/better to require nodebug to be provided earlier, rather than fixing it up after the fact like this.

Probably worth some test coverage, if possible - does this allow these transformations to be tested with opaque pointers now, or would we have to resort to unit tests to verify this new functionality?

In D103131#2835702, @kamleshbhalui wrote:

Here is what we get when we replace int with float.

$lldb-11 ./a.out 
(lldb) target create "./a.out"
Current executable set to '/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin/a.out' (x86_64).
(lldb) b main
Breakpoint 1: where = a.out`main + 4 at test.c:3:12, address = 0x0000000000400484
(lldb) p oldname
(float) $0 = 1
(lldb) p newname
(void *) $1 = 0x000000003f800000

In D103131#2835004, @kamleshbhalui wrote:

In D103131#2834744, @dblaikie wrote:

Huh, that surprises me - guess gdb favors checking the symbol first. I guess maybe it is using something that determines that that symbol comes from the file with debug info - because on a similar test case (one file without debug info, defining some global variable i, another file with debug info with a using ns::i in the global scope - printing i when stepping into the second file correctly prints the using based alias value, but stepping into the file without debug info and printing i complains about not knowing the type of that i)

How's lldb go?

lldb seems to be printing value , but I worry about type.

* thread #1, name = 'a.out', stop reason = breakpoint 1.1
    frame #0: 0x0000000000400484 a.out`main at test.c:3:12
   1    int oldname = 1;
   2    extern int newname __attribute__((alias("oldname")));
-> 3    int main(){}
(lldb) p oldname
(int) $0 = 1
(lldb) p newname
(void *) $1 = 0x0000000000000001

Huh, that surprises me - guess gdb favors checking the symbol first. I guess maybe it is using something that determines that that symbol comes from the file with debug info - because on a similar test case (one file without debug info, defining some global variable i, another file with debug info with a using ns::i in the global scope - printing i when stepping into the second file correctly prints the using based alias value, but stepping into the file without debug info and printing i complains about not knowing the type of that i)

In D95339#2833613, @mehdi_amini wrote:

In D95339#2832498, @jhenderson wrote:

In D95339#2831897, @mehdi_amini wrote:

Seems like there are only a few line changed other than the renaming? What kind of review is needed here? Just an agreement on the principle of moving these? I think it should be someone having a stake in the debug-info tests to give some sort of approval here.

Yeah, exactly. I've not had any positive confirmation that people are happy with the overall idea.

@aprantl @dblaikie maybe?

In D104342#2834119, @nickdesaulniers wrote:

In D104342#2831738, @dblaikie wrote:

In D104342#2831717, @dblaikie wrote:

Probably worth at least writing up the risk/instability in the docs for the warning (in clang) and attribute (in llvm). (don't mind if that's in this patch or a follow-up).

I would think https://clang.llvm.org/docs/DiagnosticsReference.html#wframe-larger-than would be an appropriate place to document this for -Wframe-larger-than=, but it seems this whole page is generated via TableGen. It's not clear to me how we could insert such a note.

In D104551#2833438, @kparzysz wrote:

I'm open to that, but it would likely take a while to come up with, and implement an alternative.

Would you be opposed to merging this as a workaround for the time being?

In D104551#2833161, @kparzysz wrote:

Sorry, I was incorrect in my original statement. I have fixed it.

In D104619#2831956, @nridge wrote:

Thanks for having a look!

In D104619#2831953, @dblaikie wrote:

This'll need a test case

Definitely. Do you have a suggestion for what test suite that should go into? I had a quick look but couldn't find anything that obviously exercised TypePrinter.

This'll need a test case & does the change pass all existing tests?
Also, the patch description could use more detail - it can refer to the bug for more context, but there should be enough detail in the patch title/description to understand the general purpose, etc. (and you can shorten the bug reference to "PR50774" rather than the whole bugs.llvm.org URL)

One of the concerns I'd have, for instance (have you done some broad testing of these patches on sizable code bases?) is that it wouldn't surprise me if clang had some scalability bugs/issues with very long source lines - so it might be necessary to introduce some (arbitrary?) newlines to break up the code. Though I'm not sure - no need to do that pre-emptively, but might be good to have some data that indicates whether this might be a problem or not.

In D104663#2831908, @rnk wrote:

In D104663#2831661, @dblaikie wrote:

"arguably incorrect IR" isn't a phrase that is especially comforting. It is or isn't, and should have well defined behavior (including being explicitly undefined, if that's the case). Any chance we can get a clearer statement one way or the other?

Is this an alternative to that patch that kept getting committed/reverted that I think was related to trying to unify call site and function declaration parameter attributes? Might be worth mentioning that one in this patch description for history/context?

There are other attributes that don't seem to do this kind of "try the call instruction or the function declaration", do they? so I'd be a bit hesitant about this direction. Would like to get @rnk's thoughts on this too, perhaps.

As someone who has all the context, the commit message makes sense to me. In the change you allude to, we tried to stand on the principle that a call instruction should carry all of the ABI attributes it needs, but in practice, we found that IR producers, in particular instrumentation passes, usually don't set ABI attributes on call sites. It should be a semantics-preserving transform to rinse the target of a call instruction through an identity function, but in practice, that would break all these IR producers. It would make most direct calls indirect, and the ABI attributes would no longer be correct.

If you create a review using arc's --draft (I think that's the name/spelling) it'll avoid sending email to the *-dev list, so you can create it and share it around without adding more email to the busy *-dev lists until you're ready to send it for review, btw.

What situation are you dealing with where OptBisect is created during global constructors? OptBisect uses ManagedStatic which only creates the object on first access, right? So maybe it'd be suitable to change the code so it doesn't try to access OptBisect during global construction?

In D104668#2831751, @aeubanks wrote:

In D104668#2831687, @nikic wrote:

Any thoughts on the approach here? Is the !isOpaque() ? getElementType() : nullptr pattern something we should add a helper function for? Not sure how common this is going to be outside these cast transforms.

Or possibly PointerType::hasSameElementTypeAs(PointerType *) would make sense here.

I think PointerType::hasSameElementTypeAs(PointerType *) is good, we already have weird PointerType methods that are only for the opaque pointer transition, like PointerType::getWithSamePointeeType(), that aren't used in a lot of places and are to be deleted after.

This is probably more @aaron.ballman 's wheelhouse, but for my money this seems pretty problematic - will make quoted text in compiler diagnostics weird/difficult to read, etc.

In D104342#2831733, @nickdesaulniers wrote:

In D104342#2831717, @dblaikie wrote:

I don't know that there's a good answer (in more extreme cases - like different optimization levels or CPU features, at least at Google our answer has ended up being "compile the whole program with the right CPU features, because there's no great way to support good optimizations while respecting CPU features on a per-function basis"), basically - so this was more a "heads up, this is going to be possibly unavoidably messy/unreliable on the edges".

Probably worth at least writing up the risk/instability in the docs for the warning (in clang) and attribute (in llvm). (don't mind if that's in this patch or a follow-up).

Sure. Let me land this, since we (Google & ClangBuiltLinux) have some tests and builds failing due to https://reviews.llvm.org/D103928. I will then send a follow up for us to iterate on in regards to documenting this more.

(why I think there's no solution to this: any rule (highest wins, lowest wins, mismatch fails to compile) will create surprising/problematic effects, eg: you have one TU with the function in it and some value for the attribute - a new TU could introduce a copy of the function with a higher or lower value - and whichever choice of policy would then cause problems for one case or the other case. (either enforcing a stronger warning level on code that didn't ask for it, or slackening the warning level for code that thought it was protected by the warning))

I agree. I don't think even having a function attribute in C for -Wframe-larger-than would resolve such policy issues either.

In D104342#2831703, @nickdesaulniers wrote:

In D104342#2828388, @dblaikie wrote:

In D104342#2828193, @nickdesaulniers wrote:

In D104342#2827847, @dblaikie wrote:

Another thing you might want to check is linkonce_odr functions - I guess you'll get an arbitrary choice between two linkonce_odr functions under LTO where they have different warn-stack-size? Maybe there's a way/place to merge and always pick the lower or upper value if there's one you think would be more right?

I've added an llvm-link test for this. I'm not sure it adds any signal though here; I think the answer to such a question is "don't do that."

I don't think it's as easy as "don't do that". Unless someone passes exactly the same flags to every compilation (which they won't, that's why this is being implemented as a function attribute) then it'll be really easy for an inline function in a header (say, std::vector<int>::size - something easy for two unrelated translation units to use) in two different translation units each with a different warn-stack-size flag and so to get somewhat arbitrary behavior about how that function is warned on.

Ah, ok that's which language construct can produce linkonce_odr. Fair point.

For instance: maybe the function doesn't get a warning because a copy with a higher warn-stack-size value is picked, until the translation unit using that higher value is refactored and starts using std::list instead of std::vector... and now some other TU's std::vector is picked, with a lower warn-stack-size value and breaks the build (assuming -Werror)...

This seems like a general problem perhaps with linking IR? Perhaps IRLinker::copyFunctionProto or IRLinker::mapAttributeTypes should try to do something here, though I'm not sure yet which policy would be preferred?

"arguably incorrect IR" isn't a phrase that is especially comforting. It is or isn't, and should have well defined behavior (including being explicitly undefined, if that's the case). Any chance we can get a clearer statement one way or the other?

In D104342#2828193, @nickdesaulniers wrote:

In D104342#2827847, @dblaikie wrote:

Another thing you might want to check is linkonce_odr functions - I guess you'll get an arbitrary choice between two linkonce_odr functions under LTO where they have different warn-stack-size? Maybe there's a way/place to merge and always pick the lower or upper value if there's one you think would be more right?

I've added an llvm-link test for this. I'm not sure it adds any signal though here; I think the answer to such a question is "don't do that."

@rsmith @dexonsmith @aaron.ballman - folks who might have some opinions on C++ API design. Any thoughts on this patch in general, but also in particular how the discriminator is represented in supported classes? (I'd like to avoid macros, and there are a few tradeoffs around that)

Sounds alright

Sounds OK to me.

In D104380#2826465, @DavidSpickett wrote:

This change would have been part of https://reviews.llvm.org/D103701 if I had realised that this was in fact how it worked. I separated it from the follow ons because of that and to not bury 3 lines of change that apply to all callers of these functions, in 300 lines of change to callers of SetStatus(eReturnStatusFailed). If I were bisecting a failure caused by these changes I'd appreciate the separation but there's probably not much difference.

Sounds good, if this already passes all tests/etc (ie: there actually aren't any callers passing empty strings/non-failed errors/etc)

Looks good, thanks!

In D90352#2825197, @GMNGeoffrey wrote:

In D90352#2825057, @dblaikie wrote:

Might make sense to have someone from the committee that approved the proposal sign off on these files being checked into this location (with no review of the content/quality of the files) - and perhaps to commit at that point (regardless of the content/quality of the files) - then do the actual semantic review separately.

Or if there's some part of this review that could be extracted (is there sort of a "null" implementation - maybe literally just the top level utils/bazel directory, empty, or with a README, etc) and committed with that approval, and then invested bazel-using folks could use this review thread for further discussion on the design of the bazel build files?

I'm happy to restructure this patch in that way if others would prefer (check in the README first). I did think about something like that, but I thought that non-Bazel folks might want to review some of the actual substance.

In D104342#2825357, @nickdesaulniers wrote:

In D104342#2820811, @dblaikie wrote:

what're the rules about module level attributes like this? For instance: Does this affect/hinder inlining (if the attributes don't match between the caller and callee)? Other situations that might matter?

I think you meant s/module level/function level/? That's a good question, one I had to think about a little bit. Here's my thoughts on the behavior this should exhibit, please let me know if you agree.

When using -Wframe-larger-than=<threshold> per TU, the developer wants to be alerted if any stack frame exceeds a threshold. The Linux kernel's use case is that the kernel's stack is limited to (usually) two pages (ulimit -s; typically 8KiB, but different architectures do support non-4KiB page sizes), so functions using more than 1KiB of stack are usually indicative of large objects being stack allocated that should have been heap allocated.

Currently, in C (and C with GNU extensions), there is no way to describe to the compiler function-grain specific values for -Wframe-larger-than=; rather than fine grain per function control, we only have coarse grain TU control.

So in the general case (non-LTO), we can only perform inline substitution at call sites visible from callers' definitions. Because there's no GNU C function attribute to change the current value of -Wframe-larger-than=, it's not possible for that value to differ between caller and callee. But with LTO; shit gets weird.

Suddenly now with LTO, we have cross TU (cross Module) visibility into call sites, so we can inline across TU/Module boundaries. Thus we can have an IR intermediary object with a call site where the caller's value of -Wframe-larger-than= differs from the callees! So the question is what should happen in such a case?

The extremely conservative approach which we have done in the past for certain mismatched function attributes is to simply not perform inline substitution, if we have no other options. This adds overhead to the inliner to check the XOR of attribute lists of the caller and callee for each call site.

But I *think* (and am open to sugguestions) that we should:

1. permit inline substitution
2. the caller's value of "warn-stack-size"= IR Fn Attr wins

I think this is ok because: if caller is defined in TU1 with -Wframe-larger-than= distinct from callee defined in TU2 with a different value of -Wframe-larger-than=, then we don't care what callee's value was. callee may even be DCE'd if it's inlined into a lone call site. I'd expect in such cases that callee's value was larger than caller's, in which case callee should be attributed no_inline for LTO if the tighter threshold for caller now warns. If callee's value was smaller than callers and we performed inline substitution, I think that's also perfectly fine, caller should not become "more strict."

Generally in the Linux kernel, we see a common value of -Wframe-larger-than= throughout most of the TUs, with only a few generally having a larger value to relax constraints a little. (Also, those relaxations are questionable, given the intent of -Wframe-larger-than= use in the kernel in the first place).

Let me add such a test to encode that intention; though I don't know yet what's involved/possible to implement. Let's see.

Might make sense to have someone from the committee that approved the proposal sign off on these files being checked into this location (with no review of the content/quality of the files) - and perhaps to commit at that point (regardless of the content/quality of the files) - then do the actual semantic review separately.

This might be a case of an overly reduced patch - this patch alone I'd expect to be observable in some way, and tested. If the "instring.empty()" condition never fires, then I'd expect to change that to an assert, rather than changing the semantics of it in an unobservable way.

In D104291#2823594, @shchenz wrote:

In D104291#2821581, @stuart wrote:

There is no CPlusPlus03 in LangOptions, so it is better not to merge DW_LANG_C_plus_plus_03 support with D99250.

Oh - I see, c++03 is defined in LangStandards.def an alias for c++98, as the former essentially consists of bugfixes for the latter. This loosely suggests to me that C++03 implementations are (likely to be / mostly?) conformant to C++98, but that C++98 implementations may not be fully conformant to C++03. Given this alias, it doesn't seem at all clear to me which of DW_LANG_C_plus_plus_98 and DW_LANG_C_plus_plus_03 would be the better choice, if both C++98 and C++03 must share a language tag... but I presume this has been discussed before. (It also doesn't seem clear whether it would be better to model "c++98" as an alias for "c++03".)

Yes, we don't have DW_LANG_C_plus_plus_17 and DW_LANG_C_plus_plus_20 in clang for now. I guess this is because clang does not support DWARF 6. DWARF 6 is not officially released? Once DWARF 6 is released and clang starts to support DWARF 6, I think we should add the support for DW_LANG_C_plus_plus_17 and DW_LANG_C_plus_plus_20 in the place that this patch changes.

New DWARF language codes are published ahead of the release of the next version of DWARF, so that they may be used by implementations without having to wait for new DWARF version.

It would therefore make sense to go ahead and add DW_LANG_C_plus_plus_17 and DW_LANG_C_plus_plus_20 now, without waiting, but only in the non-strict DWARF mode. (There would be the question of whether it would still make sense in the code to say "DwarfVersion >= 6" given that DWARF 6 would be otherwise unsupported... but I don't have a strong view on that question.)

I think it would be strange to check DwarfVersion >= 6 if we can not set dwarf version to 6 by anyways. Maybe we can first add the DWARF 6 support to the front end first(Need a wider discussion). This is the patch where -gdwarf-5 was introduced https://reviews.llvm.org/rG3cb592d6b60c. Seems it was also before DWARF 5 was official released.

In D104272#2823088, @zequanwu wrote:

In D104272#2820814, @dblaikie wrote:

In D104272#2820748, @aeubanks wrote:

I couldn't parse your comment, could you rephrase?

Oh, I just went and looked more - and now I see that the mangled parameter types are . separated, so the ambiguities I thought could occur (is p0i8 an opaque pointer followed by an i8 or a typed pointer-to-i8? Not actually ambiguous, because opaque pointer followed by i8 would be p0.i8 instead).

Carry on!

So, it is not ambiguous. Does the patch look good to you?

You can mark it abandoned by choosing that action just above the feedback text box

In D104272#2820748, @aeubanks wrote:

I couldn't parse your comment, could you rephrase?

what're the rules about module level attributes like this? For instance: Does this affect/hinder inlining (if the attributes don't match between the caller and callee)? Other situations that might matter?

In D104341#2820780, @ayermolo wrote:

In D104341#2820734, @dblaikie wrote:

Can you instead use the API more like the way llvm-symbolizer does - starting with the skeleton CU in the executable and it'll be able to load the dwp itself? (this codepath is already present and more tested, covers other sections like rnglists, etc, that you'll need too)

Hmm. I thought way below is it.

Only relevant code. DwCtx is context of binary.

DWPContext = DwCtx->getDWOContext(opts::DWPPath.c_str());
llvm::Optional<uint64_t> DWOId = DwarfUnit->getDWOId()
DWARFCompileUnit *DWOCU = DWPContext.get()->getDWOCompileUnitForHash(*DWOId);

Let me poke around llvm-symbolizer some more. Last time I looked I don't remember seeing low level APIs, and more high level ones.

In D104272#2820617, @zequanwu wrote:

In D104272#2820587, @dblaikie wrote:

The patch looks mostly good, but I'm not sold on "op". Can we just use "p"?

Presumably then we wouldn't know whether to parse a type after the address space or not? Unless we only support this when everything is opaque pointers? (so there can be no confusion between opaque pointers or not)

It seems what's after "{intrinsic}." is not important for IR parsing: https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/IntrinsicInst.cpp#L127.

Sounds good

Can you instead use the API more like the way llvm-symbolizer does - starting with the skeleton CU in the executable and it'll be able to load the dwp itself? (this codepath is already present and more tested, covers other sections like rnglists, etc, that you'll need too)

The patch looks mostly good, but I'm not sold on "op". Can we just use "p"?

In D98799#2820198, @ahatanak wrote:

There are several files other than CGBlocks.cpp in which FunctionDecl::Create is called to create fake FunctionDecls. Do those places have to be fixed too?

In D104082#2818281, @ahatanak wrote:

C++ non-virtual thunks and global initialization functions (_GLOBAL__sub_I_*) have only linkage names.

(generally looks like the right direction to me - sounds like @aeubanks has some good feedback/issues to address)

Looks good, thanks!

Sounds OK - though might be worth considering #including "GraphTraits.h" rather than the forward declaration - makes maintenance a bit easier later on, if default template arguments were added to GraphTraits or other changes are made.

Looks right to me

DICompositeType has a "SizeInBits" field - can that be used to specify a size of 0, instead of adding a new flag to convey this?

In D103131#2814595, @kamleshbhalui wrote:

In D103131#2814015, @dblaikie wrote:

In D103131#2811969, @kamleshbhalui wrote:

In D103131#2811220, @dblaikie wrote:

Any idea if the GDB test suite covers this functionality? I'd hope so, but maybe it doesn't.

But yeah, at the moment I don't have any great reason to avoid the imported declaration form - so happy to go with that.

Hi David,

with imported declaration patch and with current patch(in review or say gcc way) this case works ok(we can print type and value of newname)
$cat test.c
int oldname = 1;
extern int newname attribute((alias("oldname")));

but when we make newname static it works with current patch(in review or say gcc way) but it does not work with imported decl patch(https://reviews.llvm.org/D103131?id=347883).

Should we go with gcc way or am I missing something?
note: used gdb debugger.

An ideas what's happening when newname is static? Is the DWARF any different/interesting there? (since the DWARF for imported decl seems like it would have nothing to do with the linkange of the alias - since it doesn't refer to the actual alias in the object file, etc)

There not much different apart from extern has external attribute.
case 1) when newname is static

.debug_info contents:
0x00000000: Compile Unit: length = 0x00000072, format = DWARF32, version = 0x0004, abbr_offset = 0x0000, addr_size = 0x08 (next unit at 0x00000076)

0x0000000b: DW_TAG_compile_unit
              DW_AT_producer    ("clang version 13.0.0 (git@github.com:llvm/llvm-project.git 4cd7169f5517167ef456e82c6dcae669bde6c725)")
              DW_AT_language    (DW_LANG_C99)
              DW_AT_name        ("test.c")
              DW_AT_stmt_list   (0x00000000)
              DW_AT_comp_dir    ("/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin")
              DW_AT_low_pc      (0x0000000000000000)
              DW_AT_high_pc     (0x0000000000000008)

0x0000002a:   DW_TAG_variable
                DW_AT_name      ("oldname")
                DW_AT_type      (0x0000003f "int")
                DW_AT_external  (true)
                DW_AT_decl_file ("/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin/test.c")
                DW_AT_decl_line (1)
                DW_AT_location  (DW_OP_addr 0x0)

0x0000003f:   DW_TAG_base_type
                DW_AT_name      ("int")
                DW_AT_encoding  (DW_ATE_signed)
                DW_AT_byte_size (0x04)

0x00000046:   DW_TAG_variable
                DW_AT_name      ("newname")
                DW_AT_type      (0x0000003f "int")
                DW_AT_decl_file ("/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin/test.c")
                DW_AT_decl_line (2)
                DW_AT_declaration       (true)

0x00000051:   DW_TAG_imported_declaration
                DW_AT_decl_file ("/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin/test.c")
                DW_AT_decl_line (2)
                DW_AT_import    (0x00000046)
                DW_AT_name      ("newname")

0x0000005c:   DW_TAG_subprogram
                DW_AT_low_pc    (0x0000000000000000)
                DW_AT_high_pc   (0x0000000000000008)
                DW_AT_frame_base        (DW_OP_reg6 RBP)
                DW_AT_name      ("main")
                DW_AT_decl_file ("/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin/test.c")
                DW_AT_decl_line (3)
                DW_AT_type      (0x0000003f "int")
                DW_AT_external  (true)

0x00000075:   NULL

case 2) when newname is extern

.debug_info contents:
0x00000000: Compile Unit: length = 0x00000072, format = DWARF32, version = 0x0004, abbr_offset = 0x0000, addr_size = 0x08 (next unit at 0x00000076)

0x0000000b: DW_TAG_compile_unit
              DW_AT_producer    ("clang version 13.0.0 (git@github.com:llvm/llvm-project.git 4cd7169f5517167ef456e82c6dcae669bde6c725)")
              DW_AT_language    (DW_LANG_C99)
              DW_AT_name        ("test.c")
              DW_AT_stmt_list   (0x00000000)
              DW_AT_comp_dir    ("/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin")
              DW_AT_low_pc      (0x0000000000000000)
              DW_AT_high_pc     (0x0000000000000008)

0x0000002a:   DW_TAG_variable
                DW_AT_name      ("oldname")
                DW_AT_type      (0x0000003f "int")
                DW_AT_external  (true)
                DW_AT_decl_file ("/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin/test.c")
                DW_AT_decl_line (1)
                DW_AT_location  (DW_OP_addr 0x0)

0x0000003f:   DW_TAG_base_type
                DW_AT_name      ("int")
                DW_AT_encoding  (DW_ATE_signed)
                DW_AT_byte_size (0x04)

0x00000046:   DW_TAG_variable
                DW_AT_name      ("newname")
                DW_AT_type      (0x0000003f "int")
                DW_AT_external  (true)
                DW_AT_decl_file ("/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin/test.c")
                DW_AT_decl_line (2)
                DW_AT_declaration       (true)

0x00000051:   DW_TAG_imported_declaration
                DW_AT_decl_file ("/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin/test.c")
                DW_AT_decl_line (2)
                DW_AT_import    (0x00000046)
                DW_AT_name      ("newname")

0x0000005c:   DW_TAG_subprogram
                DW_AT_low_pc    (0x0000000000000000)
                DW_AT_high_pc   (0x0000000000000008)
                DW_AT_frame_base        (DW_OP_reg6 RBP)
                DW_AT_name      ("main")
                DW_AT_decl_file ("/folk/kkumar/tcllvm/llvm-build-lldb-rel/bin/test.c")
                DW_AT_decl_line (3)
                DW_AT_type      (0x0000003f "int")
                DW_AT_external  (true)

0x00000075:   NULL

Sounds good to me - couple of optional comments/questions that could be clarified in the text.

Fair enough

In D103888#2813236, @RKSimon wrote:

In D103888#2811305, @dblaikie wrote:

Sounds OK.

I wouldn't mind the places that can use op<< to use that - not sure preserving the explicit radix argument is super high value. (I think people would generally assume that's the default)
Possible we could call it to_string, is std::to_string meant to be an ADL extension point, so that other types expose a to_string in their own associated namespace, etc?

We already have a llvm::to_string(V) wrapper inside ScopedPrinter.h that uses a temp llvm::raw_string_ostream to create the string - do you think that'd be good enough?

In D103131#2811969, @kamleshbhalui wrote:

In D103131#2811220, @dblaikie wrote:

Any idea if the GDB test suite covers this functionality? I'd hope so, but maybe it doesn't.

But yeah, at the moment I don't have any great reason to avoid the imported declaration form - so happy to go with that.

Hi David,

with imported declaration patch and with current patch(in review or say gcc way) this case works ok(we can print type and value of newname)
$cat test.c
int oldname = 1;
extern int newname attribute((alias("oldname")));

but when we make newname static it works with current patch(in review or say gcc way) but it does not work with imported decl patch(https://reviews.llvm.org/D103131?id=347883).

Should we go with gcc way or am I missing something?
note: used gdb debugger.

In D104082#2812088, @ahatanak wrote:

Also, is it okay to emit the linkageName, but not the name for the helper functions? It seems okay to me, but I'm not sure.

Sounds OK.

Any idea if the GDB test suite covers this functionality? I'd hope so, but maybe it doesn't.

Does the outliner only run for commoning code, or can it run on a single function (for hot/cold reasons, or anything else)?

In D103811#2810887, @beanz wrote:

So, I realize I probably should have mentioned this earlier on the llvm-dev thread, but is this really a change we should be making the week after Phabricator officially became an unsupported project:

https://admin.phacility.com/phame/post/view/11/phacility_is_winding_down_operations/

I feel like in practice we use Phabricator and not really pre-commit email reviews, so this might just be formalizing the practice, but Phabricator not being officially maintained means that we'll likely need to move off it at some point, so this policy change would likely be short lived.

In rG937c4cffd02420607ada23d3d25078df9bf553cb#1002732, @RKSimon wrote:

This appears to change behavior - any chance of test coverage?

@dblaikie Not as far as I can work out - from what I can tell the integer types should have been legalized at that point, but preventing the implicit fall-through appeases compilers/analyzers and guarantees that we end up with a fatal error instead of a float dereference.

Looks good!

Looks good

In D103502#2801992, @jhenderson wrote:

Mostly looks good to me, but would be nice if one of the earlier reviewers gives thumbs up before we commit this patch.

Sounds good

This looks about right to me - probably follow-up on the email threads to mention this has been sent for review/link to this review?

In D98477#2800058, @scott.linder wrote:

In D98477#2798018, @dblaikie wrote:

In D98477#2797129, @scott.linder wrote:

In D98477#2787409, @dblaikie wrote:

In D98477#2783276, @scott.linder wrote:

In D98477#2780702, @dblaikie wrote:

Side thought: If this is going to use a prefix for the discriminator - could that be a base class that all types being used as union members have to derive from? No need for a macro, etc. Could use an offsetof comparison to make sure that not only is it a base class, but the first non-empty base class.

(still haven't been able to easily apply this locally - looks like it hit a conflict with c6f20d70a8c93ab3d50c9777c477fe040460a664 - though I thought arc would sync to the right version to apply the patch... not sure what's going on - might just wait to try that until the other patches have been committed)

Unfortunately having any non-static data members as part of a base class will make the alternative types non-standard-layout, which means we can't use the "common initial sequence" rule to pack them.

But would we need to use the common initial sequence rule? If the base object was at the start of the layout - a pointer to the start of the layout would be a valid pointer to the base object, yeah?

AFAIK that isn't true: we can't assume the base subobject is at the start of the layout of the derived object unless both are standard-layout.

My hope/thought was/is to use offsetof to check that the member is at the start of the object. Though, yes, for maximal portability, offsetof does require standard layout (though it seems both clang and gcc support it at least in some more cases than that: https://godbolt.org/z/Pjr6YEqMq )

In any case - even if it still leaves us with the standard layout restriction - using a base class rather than a macro+member seems like a nicer/more usable solution?

The moment we have non-static data members in both a derived class and its base class the derived class becomes non-standard-layout, though. Even just the following will not work:

struct VariantHeader {
  uint8_t Tag;
};

// SomeAlternative is not standard-layout
struct SomeAlternative : VariantHeader {
  int Foo;
};

Looks good, thanks!

In D102296#2799879, @Amir wrote:

@dblaikie: mind accepting the diff then?

Is the C source syntax already fixed? Could it use a more specific name - then maybe wouldn't need the flag to opt-in if the only places the attribute appears is where it should be propagated?