"context" as the name for this confuses me a bit - perhaps you can describe it more (maybe I'm jus tthinking about it wrong, or maybe it could benefit from a rename)

In D138247#4217189, @efriedma wrote:

The relevant text of the current Itanium ABI (which was updated in https://github.com/itanium-cxx-abi/cxx-abi/commit/d8e9d102c83f177970f0db6cc8bee170f2779bc1)

In the following contexts, however, the one-definition rule requires closure types in different translation units to "correspond":

• default arguments appearing in class definitions
• default member initializers
• the bodies of inline or templated functions
• the initializers of inline or templated variables

Could you update the references to the ABI document to use the new text?

Rename VariableTemplate classification to TemplatedVariable to match ABI wording

Remove StaticDataMember and classify static member variables of templates as variable templates

In D146986#4225192, @aaron.ballman wrote:

In D146986#4225121, @dblaikie wrote:

From the discussion on the issue:

Do we want this loosening of the restriction to apply to *only* std and the same followed by a number, or to any reserved identifier used in a module? e.g.,

module std; // error today, but will become a warning
module _Test; // error today, but do we want this to become a warning as well?

my thinking is we probably want all of these to be warnings because it'd be hard to explain why std is reserved but with a warning while _Test is reserved but with an error.

Yeah, I'd treat them equally - while we could subset the reserved names and allow implementations to only use a subset (while leaving the rest as an error for both implementations and consumers alike) that doesn't feel in keeping with the purpose of these names - to be usable by /someone/ and so necessary to allow them to be used.

(hmm - there's some discussion in the description about the fact that this error was already suppressed in "system headers" - why was that suppression inadequate for system implementation modules? (& does that suppression for reserved names risk being over-broad, since every third party library installed on a system is generally considered a "system header", even if they aren't part of the implementation?))

We currently use line markers to "enter" a system header and that's quite fragile. I mentioned we could use #pragma clang system_header, but @ChuanqiXu didn't think that was appropriate because these are not headers, they're modules, and we should have some separation between "system headers" and "system modules". (https://github.com/llvm/llvm-project/issues/61446#issuecomment-1473029776) As for being over-broad, it might be, but this is the approach we usually take (anything that's a "system header" is considered special and gets less diagnostics because the user isn't typically able to change the contents of the header file anyway).

In D146595#4225132, @aprantl wrote:

In D146595#4225048, @dblaikie wrote:

I know this is a bit of a redirection/scope creep/etc - but I'd quite like to see a solution that is likely to be usable for the "std::function" problem (stepping into std::function should allow you to reach the underlying function - but lldb currently skips any call to a std-namespaced function, I think, so you step right over the whole op() call) that could also cover the Swift needs. Though perhaps they're just sufficiently different problems that there is no generalizing here.

This patch aims at exposing an existing LLVM IR & DWARF feature in clang.

From the discussion on the issue:

Do we want this loosening of the restriction to apply to *only* std and the same followed by a number, or to any reserved identifier used in a module? e.g.,

module std; // error today, but will become a warning
module _Test; // error today, but do we want this to become a warning as well?

my thinking is we probably want all of these to be warnings because it'd be hard to explain why std is reserved but with a warning while _Test is reserved but with an error.

Thanks!

I know this is a bit of a redirection/scope creep/etc - but I'd quite like to see a solution that is likely to be usable for the "std::function" problem (stepping into std::function should allow you to reach the underlying function - but lldb currently skips any call to a std-namespaced function, I think, so you step right over the whole op() call) that could also cover the Swift needs. Though perhaps they're just sufficiently different problems that there is no generalizing here.

Looks good, thanks!

Hmm, the way we're reading this doesn't seem like it /quite/ makes sense - we read the offsets that are encoded in the "symbol table" but then we read the constant pool CU vectors directly based on the number of unique offsets in the symbol table.

In D146595#4218040, @augusto2112 wrote:

What would happen if, instead, these trampolining functions were annotated nodebug? I guess then you wouldn't have the top level one as an entry point for the user, as there would be no debug info for it?

These trampoline functions can call other functions as part of their setup, so annotating them with nodebug wouldn't guarantee that the debugger would be able to step through to the correct place. Additionally, you wouldn't be able to refer to them in LLDB expression evaluation either.

Oh, that's a bit worrying - that these aren't trampolines in the sort of lower-level sense, only in a high level sense.

Is the ability to debug these functions under some debugger flag ("show me intermediate steps/implementation details") a priority? Would it be OK if that were only available with a rebuild (ie: maybe didn't need to be represented in DWARF - this would also allow debug info to be smaller & not having to describe these transparent/implementation detail functions)?

You mean step in the trampoline? It'd be nice, but not absolutely required. The main issue would be not being able to evaluate expressions though.

In D146466#4218011, @efriedma wrote:

I think the reason "recoverable" ubsan causes trouble is that it introduces branches that subsequent optimizations can abuse. So without ubsan, we just have an udiv instruction. With ubsan, we conveniently have a branch on exactly the condition that would make the udiv undefined, so we can easily prove control flow doesn't continue after the ubsan handler. Subsequent optimizations take advantage of that, so ubsan "breaks" code. (So the code was never actually correct according to the semantic model, but it was broken in a way the compiler is less likely optimize.)

In D146595#4217855, @augusto2112 wrote:

Should there be some check that that symbol exists? (the current test uses the unmangled name "bar" for instance - which gives the wrong impression?)

You mean as part of the attribute validation?

(recoverable feels like a bit of a distraction here? recoverable just means you've asked ubsan not to trap/stop on failure - but to let the program continue and do whatever it would've done without the sanitizer enabled - sometimes that's crash/trap anyway, sometimes it's something less bad... but that's all that's being asked for: "keep going/do whatever you'd do without the sanitizer enabled, rather than hard stop as soon as the sanitizer detects a problem" - no, we shouldn't recover differently/more safely with sanitizers enabled (don't want to create a language variant/encourage people to build incorrect programs with sanitizers and run them that way because "it works"))

fix DWARF5 reference to DWARF4 for the test case

What about function overloading & namespaces? Is the debugger expected to figure out which bar function the DWARF/user is referring to?

The symbol name that's passed in the attribute is supposed to be the mangled name, so there shouldn't be any ambiguity to resolve.

The rationale is: dwim-print doesn't always use expression evaluation, it prefers to use frame variable where possible. In the future it could be expanded, for example to print register as well. Because dwim-print doesn't always use expression, there isn't always a persistent result.

Ping

In D146466#4213814, @nickdesaulniers wrote:

In D146466#4207915, @efriedma wrote:

I'm concerned about the potential for false positives:

• If a user doesn't mark a function noreturn, but it doesn't actually ever return.

Ok, but a user can fix that in their code. Also, I think we could possibly help by adding diagnostics for this. Unconditionally calling a noreturn function or having an obviously infinite loop should trigger such a diagnostic with note encouraging the user to mark their function noreturn as well?

So looking at the DWARF spec for this - I see what it's going for, but yeah, I wouldn't want to use a textual representation either at the source or bitcode level, ideally... - perhaps this should just be implemented in LLVM when something's being lowered to a trampoline we could emit DWARF that describes that, without needing source annotations? Or are there situations where it depends on how the user wrote the code as to how we want the debugger to dehave?

Yeah, this seems a bit burdensome for both the frontend (encoding more information, the author has to duplicate the name of the implementation function so there's a risk they get out of sync?) and the debugger (is the debugger expected to jump directly and not call the implementation? Is it expected to continue into the implementation and break at the function? What if the function isn't called, or there's more than one call in the function, etc?)...

Looks good, thanks!

Yeah, we already have the return type warning and UBSan for the dynamic version of this check - seems OK? That a function lowers to unreachable isn't a bug - there's a ton of them in LLVM, quite intentionally as unimplemented functions that should never be called.

No, my intent was not to use UnknownLocations as a proxy for CV, but I think it is safe not to fold line 0 instructions into a scope range when UnknownLocations is enabled. AFAIK, when UnknownLocations is enabled line 0 entries are emitted in the debug information. In my opinion, Folding these instructions when the switch is enabled could break stuff.

Would std::distance suffice?

I don't think of this as a performance regression for users though - this functionality's never really "shipped" so we get to choose what the baseline is.

In rG35fd37177b9b201f26390fe963767be548c8c2e9#1184231, @MaskRay wrote:

Is this worth a unit test?

If you consider the attached test case (llvm/test/DebugInfo/X86/contiguous-lexical-block.ll) you can see that the lexical block is fragmented into ranges due to the presence of an instruction having line number 0. This I believe is a bug and If the same test case is run on windows, Visual Studio' can't reliably print variable "i" because CodeView does not provide a mechanism for describing a lexical block with more than one address ranges. In order to prevent the line 0 instructions from breaking up a lexical block, in this patch, we fold the line zero machine instructions into the range.

In D146061#4193697, @kuhar wrote:

This is also consistent with how new C++ range functions are defined, e.g., https://en.cppreference.com/w/cpp/ranges/size that calls member .size when available and does iterator subtraction otherwise.

In D146061#4193671, @kuhar wrote:

In D146061#4193557, @nikic wrote:

I also considered detecting member contains and triggering a
static_assert instead, but decided against it because it's just as easy
to do the right thing and call .contains.

I think I would prefer this option, so we keep the ability to distinguish O(1) contains from O(n) is_contained at a glance.

The way I was thinking about this is that usually the C++ standard specifies algorithms in terms of worst-case complexity and anything better is a bonus. In a completely generic context, as a user of is_contained my expectation would be that it's O(n) or better.

This situation is a bit more complicated because C++20 is added member contains functions to (unordered_)set/map, so a static assertion would cause compilation failures with some compiler flags and not the other ones. IMO this is not a huge deal but probably annoying because I'd expect the Hyrum law to apply and API usages of is_contained with these containers to creep in over time. The current patch makes this API instability problem better , as there'll be no need to have ways to essentially do the same thing.

In D144844#4193727, @iains wrote:

In D144844#4193568, @dblaikie wrote:

Seem to recall @iains and others were concerned about the number of modules flags - this one I'd be inclined to suggest we shouldn't add if possible. If one way or the other is generally better - we should, at least initially, dictate that behavior entirely until someone can demonstrate divergent use cases that seem reasonable to support but must have different behavior here.

Agreed, [IMO, at least] we have a very large set of modules options, and expanding that should only be done if no sensible alternative can be found (we are already seeing users getting confused about which ones apply in each case).

A second point here - that (once we have the ability to generate object and PCM in the same compilation) that we will move to elide the function bodies for non-inline and non-dependent cases from the PCM, so that this problem will magically "go away" (to restore the current behaviour, we'd then be using some optimisation control to avoid the elision, I suppose).

Looks good, thanks!

In D146061#4193581, @dblaikie wrote:

In D146061#4193557, @nikic wrote:

I also considered detecting member contains and triggering a
static_assert instead, but decided against it because it's just as easy
to do the right thing and call .contains.

I think I would prefer this option, so we keep the ability to distinguish O(1) contains from O(n) is_contained at a glance.

Yeah, that's fair - I'm OK with that too/see the value there. Only issue is in generic code where you might not know the type of a container.

In D146061#4193557, @nikic wrote:

I also considered detecting member contains and triggering a
static_assert instead, but decided against it because it's just as easy
to do the right thing and call .contains.

I think I would prefer this option, so we keep the ability to distinguish O(1) contains from O(n) is_contained at a glance.

Seem to recall @iains and others were concerned about the number of modules flags - this one I'd be inclined to suggest we shouldn't add if possible. If one way or the other is generally better - we should, at least initially, dictate that behavior entirely until someone can demonstrate divergent use cases that seem reasonable to support but must have different behavior here.

Looks good

Looks alright - thanks! :)

In D143985#4188825, @krisb wrote:

@dblaikie thank you for looking at this! Hope this will be the final and successful iteration of the patchset.

In D143985#4185516, @dblaikie wrote:

Hmm - have you checked/could you check if debuggers (gdb and lldb at least) care about where a namespace-scoped imported entity appears in the DWARF? It looks like this patch would move namespace-scoped imported entities to the end of the DWARF, and I could imagine an implementation trying to do name lookup might try to consider only the imported entities that appear lexically before in the DWARF? (admittedly we haven't put enough information in the IR to get these things in the /correct/ place, but maybe before is more likely to be correct than after) - though of course the same bug might exist in function-local imported entities (& other function local names) - and similarly we don't have enough info or ways to insert things in the right order anyway...

I haven't consulted with the debuggers code itself, but a simple example showed that neither gdb nor lldb have problems with finding imported entities if the order gets changed:

In D144337#4185295, @ykhatav wrote:

In D144337#4183551, @dblaikie wrote:

Could you describe why this relates to/depends on the "UnknownLocations" flag? I'm not following from the description - we could still describe these locations as being at line zero, while including them in lexical scopes to avoid fragmented ranges, I think?

The reason why we want to check for “UnknownLocations” flag is that this flag is used to check if we want to add line 0 entries in the line table (in DwarfDebug.cpp) and thereby controlling whether or not line zero instructions are represented in the debug information. If we don’t check for this flag and skip the line 0 entries while extracting lexical scopes there will be a mismatch in the line table entries and lexical scopes.

In D144008#4188878, @krisb wrote:

@dblaikie

In D144008#4185551, @dblaikie wrote:

@krisb - how's this handle cases of old IR being used in a newer compiler/linker?

There is no backward compatibility, old IR may cause DwarfDebug crash (it asserts that old IR is no longer supported) or miscompilation.
Implementing AutoUpgrade seems to be quite easy, but as far as I can see almost all the changes in debug metadata before were done w/o backward compatibility, so I haven't considered it yet.

In D144007#4188854, @krisb wrote:

In D144007#4185548, @dblaikie wrote:

I guess this one's a bit clearer than the type one about why changing the order is important for getting local entities correctly scoped - local static is a global variable for the debug info and IR, so we need to emit the scopes before we'll know where to put the globals, etc.

The motivation for this patch is mostly the same as for types:

• unify/simplify emission of local and non-local entities getting everything done in one place (in our case DwarfDebug::endModule()). Keeping 'non-local' globals emission in DwarfDebug::beginModule() (while 'local' globals should be emitted not earlier than DwarfDebug::endModule()) would make the implementation much more complicated as both 'local' and 'global' globals emission depends on GVMap.
• make sure IR/debug metadata would not be changed after DWARF entities get created.

In D144005#4188836, @krisb wrote:

In D144005#4185541, @dblaikie wrote:

Change seems OK - but might be nice to know a bit more about why this is helpful for correcting function-local entities?

Strictly speaking, moving emission of non-local types isn't required to get local types worked. This is mostly done for the sake of unification/simplification since we are stopping emitting anything in DwarfDebug::beginModule(). There is another motivation point, but it is a bit speculative: we may get IR (as well as debug metadata) changed after DwarfDebug::beginModule(), and if this happened, emitted debug info would be out-of-date. I've found such a case only once, and considered it rather a bug (see for details https://reviews.llvm.org/D113653). I'm not aware of any other cases, but it's still possible to change debug metadata after DwarfDebug::beginModule(), so it seems much safer to emit DWARF at a time when we do not expect any changes in IR (i.e. DwarfDebug::endModule()).

Yeah, can't say this had occurred to me - but totally makes sense/reckon it's OK. Any reason to limit this to lldb? I'd expect it'd probably "Just Work(tm)" on any DWARF consumer?

Since this changes what IR goes where - are there concerns you folks have for auto-upgrade/cross compatibility?

I guess this one's a bit clearer than the type one about why changing the order is important for getting local entities correctly scoped - local static is a global variable for the debug info and IR, so we need to emit the scopes before we'll know where to put the globals, etc.

Change seems OK - but might be nice to know a bit more about why this is helpful for correcting function-local entities?

& also - if we're moving local imported entities into the function-local retainedNodes list, do we need to change this code that should just be left constructing global/namespace-scoped imported entities? Perhaps we can leave this code/ordering alone?

Hmm - have you checked/could you check if debuggers (gdb and lldb at least) care about where a namespace-scoped imported entity appears in the DWARF? It looks like this patch would move namespace-scoped imported entities to the end of the DWARF, and I could imagine an implementation trying to do name lookup might try to consider only the imported entities that appear lexically before in the DWARF? (admittedly we haven't put enough information in the IR to get these things in the /correct/ place, but maybe before is more likely to be correct than after) - though of course the same bug might exist in function-local imported entities (& other function local names) - and similarly we don't have enough info or ways to insert things in the right order anyway...

Could you describe why this relates to/depends on the "UnknownLocations" flag? I'm not following from the description - we could still describe these locations as being at line zero, while including them in lexical scopes to avoid fragmented ranges, I think?

In D144503#4179141, @zero9178 wrote:

In D144503#4179099, @kuhar wrote:

In D144503#4179026, @dblaikie wrote:

I think it's the InputIterator (ForwardIterators must be InputIterators) requirement is the one that makes value returns not possible in practice because they're required to support i->m as being equivalent to (*i).m`, which isn't possible, as far as I recall/understand without backing storage to return a pointer to?

Oh, and ForwardIterator also says (on cppreference):

Let T be the value type of It. The type std::iterator_traits<It>::reference must be either
  T& or T&& (since C++11) if It satisfies LegacyOutputIterator (It is mutable), or
  const T& or const T&& (since C++11) otherwise (It is constant),
(where T is the type denoted by std::iterator_traits<It>::value_type)

David and I talked offline and this interpretation of the standard is probably the correct one. To conform to the iterator requirements, we would have to store something like std::optional<TupleOfReferences> inside zip_common and materialize it on the first dereference. This would be a proper fix for all zip functions, but it doesn't seem like any part of llvm relies on reference being an actual reference right now, so it doesn't seem like a blocker.

I will update all uses of for (auto &it : enumerate(...)) in a separate revision before landing this and leave a FIXME for the reference type issues.

I think its also fair to say that this is somewhat a historical mistake that was never properly followed (see std::vector<bool>) which is probably why this requirement was dropped in C++20 concepts for forward iterators: https://en.cppreference.com/w/cpp/iterator/forward_iterator

I don't totally object - though, for myself (I think this was discussed previously, but I don't fully understand the refutation/disagreement) I think I'd prefer to expose the compositions themselves and probably not have a special return type, so the end result is:

for (auto &[index, v1, v2] : zip(index{}, r1, r2))

And drop "enumerate" entirely? It doesn't seem like it adds enough - though I guess having a name for it is useful. *shrug*

I think it's the InputIterator (ForwardIterators must be InputIterators) requirement is the one that makes value returns not possible in practice because they're required to support i->m as being equivalent to (*i).m`, which isn't possible, as far as I recall/understand without backing storage to return a pointer to?

In D145499#4175662, @avl wrote:

In D145499#4175487, @dblaikie wrote:

Sounds OK, though might be nice to avoid having these different ways of using (with/without DWARFUnit) and find some way to unify them (have some more general abstraction over DWARFUnit that can be provided by all users of this API or something)

something like this?

diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFFormValue.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFFormValue.h
index fa47621265f7..b13f5ffdf7f4 100644
--- a/llvm/include/llvm/DebugInfo/DWARF/DWARFFormValue.h
+++ b/llvm/include/llvm/DebugInfo/DWARF/DWARFFormValue.h
@@ -77,10 +77,7 @@ public:
   dwarf::Form getForm() const { return Form; }
   uint64_t getRawUValue() const { return Value.uval; }
 
-  /// Returns true if current value is of \p FC form class.
-  /// If current value does not have reference to original DWARFUnit
-  /// then \p DwarfVersion might be used to check form class of value.
-  bool isFormClass(FormClass FC, uint16_t DwarfVersion = 3) const;
+  bool isFormClass(FormClass FC) const;
   const DWARFUnit *getUnit() const { return U; }
   void dump(raw_ostream &OS, DIDumpOptions DumpOpts = DIDumpOptions()) const;
   void dumpSectionedAddress(raw_ostream &OS, DIDumpOptions DumpOpts,
@@ -352,6 +349,9 @@ toBlock(const std::optional<DWARFFormValue> &V) {
   return std::nullopt;
 }
 
+bool doesFormBelongToClass(dwarf::Form Form, DWARFFormValue::FormClass FC,
+                                 uint16_t DwarfVersion);
+
 } // end namespace dwarf
 
 } // end namespace llvm
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp b/llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp
index fd94c8a7ffbb..5ea0aa63c218 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp
@@ -213,41 +213,8 @@ bool DWARFFormValue::skipValue(dwarf::Form Form, DataExtractor DebugInfoData,
   return true;
 }
 
-bool DWARFFormValue::isFormClass(DWARFFormValue::FormClass FC,
-                                 uint16_t DwarfVersion) const {
-  // First, check DWARF5 form classes.
-  if (Form < ArrayRef(DWARF5FormClasses).size() &&
-      DWARF5FormClasses[Form] == FC)
-    return true;
-  // Check more forms from extensions and proposals.
-  switch (Form) {
-  case DW_FORM_GNU_ref_alt:
-    return (FC == FC_Reference);
-  case DW_FORM_GNU_addr_index:
-    return (FC == FC_Address);
-  case DW_FORM_GNU_str_index:
-  case DW_FORM_GNU_strp_alt:
-    return (FC == FC_String);
-  case DW_FORM_LLVM_addrx_offset:
-    return (FC == FC_Address);
-  default:
-    break;
-  }
-
-  if (FC == FC_SectionOffset) {
-    if (Form == DW_FORM_strp || Form == DW_FORM_line_strp)
-      return true;
-    // In DWARF3 DW_FORM_data4 and DW_FORM_data8 served also as a section
-    // offset. If we don't have a DWARFUnit, default to the old behavior.
-    if (Form == DW_FORM_data4 || Form == DW_FORM_data8) {
-      if (U)
-        return U->getVersion() <= 3;
-
-      return DwarfVersion <= 3;
-    }
-  }
-
-  return false;
+bool DWARFFormValue::isFormClass(DWARFFormValue::FormClass FC) const {
+  return doesFormBelongToClass(Form, FC, U ? U->getVersion() : 3);
 }
 
 bool DWARFFormValue::extractValue(const DWARFDataExtractor &Data,
@@ -783,3 +750,36 @@ DWARFFormValue::getAsFile(DILineInfoSpecifier::FileLineInfoKind Kind) const {
   }
   return std::nullopt;
 }
+
+bool doesFormBelongToClass(dwarf::Form Form, DWARFFormValue::FormClass FC,
+                                 uint16_t DwarfVersion) {
+    // First, check DWARF5 form classes.
+    if (Form < ArrayRef(DWARF5FormClasses).size() &&
+        DWARF5FormClasses[Form] == FC)
+      return true;
+    // Check more forms from extensions and proposals.
+    switch (Form) {
+    case DW_FORM_GNU_ref_alt:
+      return (FC == DWARFFormValue::FC_Reference);
+    case DW_FORM_GNU_addr_index:
+      return (FC == DWARFFormValue::FC_Address);
+    case DW_FORM_GNU_str_index:
+    case DW_FORM_GNU_strp_alt:
+      return (FC == DWARFFormValue::FC_String);
+    case DW_FORM_LLVM_addrx_offset:
+      return (FC == DWARFFormValue::FC_Address);
+    default:
+      break;
+    }
+
+    if (FC == DWARFFormValue::FC_SectionOffset) {
+      if (Form == DW_FORM_strp || Form == DW_FORM_line_strp)
+        return true;
+      // In DWARF3 DW_FORM_data4 and DW_FORM_data8 served also as a section
+      // offset.
+      if (Form == DW_FORM_data4 || Form == DW_FORM_data8)
+        return DwarfVersion <= 3;
+    }
+
+    return false;
+}

Sounds good to me - wouldn't even mind if you wanted to implement a generic abstraction, less<N> and implement less_first/second as aliases of that type.

Sounds OK, though might be nice to avoid having these different ways of using (with/without DWARFUnit) and find some way to unify them (have some more general abstraction over DWARFUnit that can be provided by all users of this API or something)

GCC bug hasn't got much traction, so I pinged it.

What does it mean for it to be per-function? Can it vary at all between functions in the same module? (if not, why is it per-function?)

It governs whether or not that function's text relocations are paired with R_RISCV_RELAX to tell the linker it can relax them.

Because the enumerate_result returned on dereference is a temporary, enumeration result can no longer be used through an lvalue ref.

Yep, looks good. (interestingly, this implementation might be missing some functionality for Split DWARF - in a Split DWARF build I didn't see any perf improvement, because I think there was no perf degredation - because this feature isn't properly implemented for Split DWARF, I'd guess)

Sure, sounds good. (maybe document the history of how we ended up with duplicate functionality/different names for the same thing? (I haven't checked the history - if you introduced the new names, maybe link to where they were introduced and/or summarize why they were introduced, etc))

In D145249#4167428, @tmatheson wrote:

@mehdi_amini can you explain what the original reasoning behind this restriction was for Support, and whether we need to make the same restriction for TargetParser now that it's split out?

"But when would you have a completely empty diagnostic message", you ask dear reader?
That is when there is an empty "#warning" in code.

FWIW, there's a drawback here: Now llvm-profdata is missing test coverage in the llvm subproject. Someone making changes would reasonably expect that "check-llvm" would be adequate to test their changes. So we do try pretty hard to keep tests for code in the same subproject as the code itself. (but yes, also the llvm subproject can't/shouldn't depend on compiler-rt)

Does this need a flag? The patch describes doing this for CodeView? Could/should it be done unconditionally for CV?

In D145258#4168410, @mehdi_amini wrote:

In D145258#4168143, @dblaikie wrote:

I would've thought it was necessary to alwayslink them because maybe they use some kind of auto-registration system into clang-tidy, so without alwayslink your clang-tidy binary wouldn't link any of them in? But I could be wrong, maybe they're explicitly referenced in the clang-tidy binary?

There is no always-link in cmake as far as I know, so we should not need any in Bazel either in the LLVM project hopefully.

In D145077#4168209, @dblaikie wrote:

In D145077#4162110, @aprantl wrote:

I originally was hoping we wouldn't have to introduce a new attribute for this, but it looks like there are legitimate concerns about the alternatives, so in that sense this looks good!

Out of curiosity, what were the other existing attributes that were considered? (would be curious to have them written down here to explain/add strength to the motivation to add a new extension attribute)

In D145077#4162110, @aprantl wrote:

I originally was hoping we wouldn't have to introduce a new attribute for this, but it looks like there are legitimate concerns about the alternatives, so in that sense this looks good!

I would've thought it was necessary to alwayslink them because maybe they use some kind of auto-registration system into clang-tidy, so without alwayslink your clang-tidy binary wouldn't link any of them in? But I could be wrong, maybe they're explicitly referenced in the clang-tidy binary?

In D145051#4166919, @uabelho wrote:

In D145051#4166656, @nikic wrote:

I think what we can do instead is to mark MemorySSA as preserved if only debuginfo intrinsics have been removed. (This is safe, MSSA explicitly never tracks debuginfo intrinsics.)

Does everyone agree this is the way forward then? If so I can put up a patch doing that on Monday.

In D145199#4167966, @MaskRay wrote:

remove offset (since it's always zero for the STT_SECTION symbol)

In D140478#4165992, @jrtc27 wrote:

In D140478#4165988, @dblaikie wrote:

In D140478#4164792, @RamNalamothu wrote:

@dblaikie

As I already replied earlier, there is not enough context to reach the MachineFunction from the symbol or determine if relaxation is enabled for the given address range.

Then won't this make the wrong decisions if the value varies between functions/subtargets?

Also, at the moment, the -mrelax/-mno-relax affects the entire compilation unit for the RISC-V.

Then wouldn't these be target features, rather than subtarget features? My understanding is that subtarget features generally can vary by function, whereas target features cannot.

Even if the command line flags don't support it, we could probably make a simple IR example with different relaxation per function? & so we probably should be able to handle that case.

It's a per-function feature that has implications for other functions in the same section (deleting code in one function at link time will shunt around the code in the rest of the section).

In D140478#4164792, @RamNalamothu wrote:

@dblaikie

As I already replied earlier, there is not enough context to reach the MachineFunction from the symbol or determine if relaxation is enabled for the given address range.