[lldb-dev] Extern globals showing up as locals

[Sam McCall via lldb-dev]

I'm seeing particular globals spuriously being reported as local variables:
they're returned by SBFrame::GetVariables(true, true, false, true) because
their scope is eValueTypeVariableLocal.

I've tracked down the problem but don't know DWARF well enough to determine
the right fix. Anyone want to help me out?

--

I can reproduce with the following program:

struct Obj{};
namespace N { extern Obj& o; }
using N::o;
int main(){}


When building with clang -g, lldb gives

(lldb) frame v -g -s

LOCAL: (Obj &) N::o = 

(lldb) target v -s

LOCAL: (Obj &) N::o = 

It seems confused about whether it's local or global.

readelf --dump-debug has the following info for the variable:
<2><31>: Abbrev Number: 3 (DW_TAG_variable)
<32>   DW_AT_name: (indirect string, offset: 0x51): o
<36>   DW_AT_type: <0x42>
<3a>   DW_AT_external: 1
<3a>   DW_AT_decl_file   : 1
<3b>   DW_AT_decl_line   : 2
<3c>   DW_AT_declaration : 1
<3c>   DW_AT_location: 0 byte block:()
<3d>   DW_AT_linkage_name: (indirect string, offset: 0x57): _ZN1N1oE
It seems that the zero-length location is a problem: SymbolFileDWARF.cpp
thinks a variable without a valid location must be static or local.

FWIW, gdb on the clang-built binary shows N::o as an optimized-out global,
which seems fine. gcc generates debug info that omits the DW_AT_location,
which causes gdb to hide it completely I think.

Cheers, Sam
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

[lldb-dev] Bug in CommandObjectThreadUntil

[Zachary Turner via lldb-dev]

CommandObjectThreadUntil::DoExecute() has the following code:

for (size_t i = 0; i < num_args; i++) {
  uint32_t line_number;
  line_number =
StringConvert::ToUInt32(command.GetArgumentAtIndex(0),
UINT32_MAX);
  if (line_number == UINT32_MAX) {
result.AppendErrorWithFormat("invalid line number: '%s'.\n",
 command.GetArgumentAtIndex(0));
result.SetStatus(eReturnStatusFailed);
return false;
  } else
line_numbers.push_back(line_number);
}

1) It looks to me like this is supposed to be GetArgumentAtIndex(i) instead
of GetArgumentAtIndex(0).  Is this in fact a bug?

2) The ability for this command to take multiple line numbers is not
documented.  Should it be?
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

[lldb-dev] Bug in OptionValuePathMappings::SetValueFromString

[Zachary Turner via lldb-dev]

In the switch / case handler for eVarSetOperationRemove, there is the
following code:

  if (num_remove_indexes) {
// Sort and then erase in reverse so indexes are always valid
std::sort(remove_indexes.begin(), remove_indexes.end());
for (size_t j = num_remove_indexes - 1; j < num_remove_indexes;
++j) {
  m_path_mappings.Remove(j, m_notify_changes);
}
  }

Should the line that calls Remove() not be like this:

  m_path_mappings.Remove(*remove_indexes[j]*, m_notify_changes);

?  What effect will this have on LLDB?
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

Re: [lldb-dev] Bug in OptionValuePathMappings::SetValueFromString

[Zachary Turner via lldb-dev]

Also, I just noticed the loop only runs one time, so it appears equivalent
to

m_path_mappings.Remove(remove_indexes.size() - 1, m_notify_changes);

which seems completely wrong

On Fri, Nov 18, 2016 at 1:05 PM Zachary Turner  wrote:

> In the switch / case handler for eVarSetOperationRemove, there is the
> following code:
>
>   if (num_remove_indexes) {
> // Sort and then erase in reverse so indexes are always valid
> std::sort(remove_indexes.begin(), remove_indexes.end());
> for (size_t j = num_remove_indexes - 1; j < num_remove_indexes;
> ++j) {
>   m_path_mappings.Remove(j, m_notify_changes);
> }
>   }
>
> Should the line that calls Remove() not be like this:
>
>   m_path_mappings.Remove(*remove_indexes[j]*, m_notify_changes);
>
> ?  What effect will this have on LLDB?
>
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

Re: [lldb-dev] Bug in CommandObjectThreadUntil

[Jim Ingham via lldb-dev]

Obviously a bug.  Fixed in r287386.

Jim

> On Nov 18, 2016, at 11:47 AM, Zachary Turner via lldb-dev 
>  wrote:
> 
> CommandObjectThreadUntil::DoExecute() has the following code:
> 
> for (size_t i = 0; i < num_args; i++) {
>   uint32_t line_number;
>   line_number = StringConvert::ToUInt32(command.GetArgumentAtIndex(0),
> UINT32_MAX);
>   if (line_number == UINT32_MAX) {
> result.AppendErrorWithFormat("invalid line number: '%s'.\n",
>  command.GetArgumentAtIndex(0));
> result.SetStatus(eReturnStatusFailed);
> return false;
>   } else
> line_numbers.push_back(line_number);
> }
> 
> 1) It looks to me like this is supposed to be GetArgumentAtIndex(i) instead 
> of GetArgumentAtIndex(0).  Is this in fact a bug?
> 
> 2) The ability for this command to take multiple line numbers is not 
> documented.  Should it be?
> ___
> lldb-dev mailing list
> lldb-dev@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

Re: [lldb-dev] [llvm-dev] [RFC] Runtime checks for ABI breaking build of LLVM

[Mehdi Amini via lldb-dev]

Hi,

I had to revert it, because it breaks building LLDB on MacOS.

It turns out it would break any client that is including llvm-config.h but not 
linking to libLLVMSupport.
So either:

- we shouldn’t allow to include llvm-config.h without linking to LLVM, in which 
case I need to look a bit closer as of why lldb includes this header in a 
context where they don’t link LLVM.
- we should allow to include llvm-config.h without linking to LLVM (at least 
libSupport). In which case we need a new solution for this feature: it can be 
to use another header dedicated to this flag, that would be included only from 
headers that contain the ABI break.

— 
Mehdi



> On Nov 16, 2016, at 12:12 PM, Mehdi Amini via llvm-dev 
>  wrote:
> 
>> 
>> On Nov 16, 2016, at 12:05 PM, Jonathan Roelofs > > wrote:
>> 
>> 
>> 
>> On 11/16/16 11:48 AM, Mehdi Amini via llvm-dev wrote:
>>> Hi all,
>>> 
>>> An issue that come up from time to time and has cost hours for debug for
>>> many of us and users of LLVM is that an assert build isn’t ABI
>>> compatible with a release build.
>>> 
>>> The CMake flags that controls this behavior is LLVM_ABI_BREAKING_CHECKS (
>>> 
>>> *LLVM_ABI_BREAKING_CHECKS*:STRING
>>>Used to decide if LLVM should be built with ABI breaking checks or
>>>not. Allowed values
>>>are WITH_ASSERTS (default), FORCE_ON and FORCE_OFF.  WITH_ASSERTS turns
>>>on ABI breaking checks in an assertion enabled
>>>build.  FORCE_ON (FORCE_OFF) turns them on (off) irrespective of
>>>whether normal (NDEBUG-based) assertions are enabled or not. A
>>>version of LLVM built with ABI breaking checks is not ABI compatible
>>>with a version built without it.
>>> 
>>> 
>>> I propose to add a runtime check to detect when we have incorrectly
>>> setup build.
>>> 
>>> The idea is that every translation unit that includes such header will
>>> get a weak definition of a symbol with an initializer calling the
>>> runtime check. The symbol name would be different if the ABI break is
>>> enabled or not.
>> 
>> Can it be made into a link-time check instead? I'm imagining something like:
> 
> I’d love to, but didn’t find a universal solution unfortunately :(
> 
>>  #if LLVM_ENABLE_ABI_BREAKING_CHECKS
>>  extern int EnableABIBreakingChecks;
>>  __attribute__((weak)) int *VerifyEnableABIBreakingChecks = 
>> &EnableABIBreakingChecks;
>>  #else
>>  extern int DisableABIBreakingChecks;
>>  __attribute__((weak)) int *VerifyDisableABIBreakingChecks = 
>> &VDisableABIBreakingChecks;
>>  #endif
>> 
>> in llvm-config.h.cmake, and:
>> 
>>  #if LLVM_ENABLE_ABI_BREAKING_CHECKS
>>  int EnableABIBreakingChecks;
>>  #else
>>  int DisableABIBreakingChecks;
>>  #endif
>> 
>> in Error.cpp.
>> 
>> Then it'll only link if Error.cpp's TU's setting of 
>> LLVM_ENABLE_ABI_BREAKING_CHECKS matches that of the TU that includes 
>> llvm-config.h
> 
> It seems that this work, I thought I tried exactly this but got lost on the 
> whiteboard at some point!
> 
> Maybe because one drawback that I tried to avoid is that the export-list of a 
> LLVM dylib would depend on the value of LLVM_ENABLE_ABI_BREAKING_CHECKS with 
> this.
> 
> Thanks,
> 
> — 
> Mehdi
> 
> 
> 
> 
> 
>> 
>> 
>>> 
>>> The runtime check maintains two boolean to track if it there is in the
>>> image at least a translation unit for each value of this flag. If both
>>> flags are set the process is aborted.
>>> 
>>> The cost is *one* static initializer per DSO (or per image I believe).
>>> 
>>> A straw-man patch (likely not windows compatible because of weak) is:
>>> 
>>> diff --git a/llvm/include/llvm/Config/llvm-config.h.cmake
>>> b/llvm/include/llvm/Config/llvm-config.h.cmake
>>> index 4121e865ea00..4274c942d3b6 100644
>>> --- a/llvm/include/llvm/Config/llvm-config.h.cmake
>>> +++ b/llvm/include/llvm/Config/llvm-config.h.cmake
>>> @@ -80,4 +80,18 @@
>>> /* LLVM version string */
>>> #define LLVM_VERSION_STRING "${PACKAGE_VERSION}"
>>> 
>>> +
>>> +#ifdef __cplusplus
>>> +namespace llvm {
>>> +bool setABIBreakingChecks(bool Enabled);
>>> +__attribute__((weak))
>>> +#if LLVM_ENABLE_ABI_BREAKING_CHECKS
>>> +bool
>>> +ABICheckEnabled = setABIBreakingChecks(true);
>>> +#else
>>> +bool ABICheckDisabled = setABIBreakingChecks(true);
>> 
>> Do you mean `false` here ^ ?
>> 
>>> +#endif
>>> +}
>>> +#endif
>>> +
>>> #endif
>>> diff --git a/llvm/lib/Support/Error.cpp b/llvm/lib/Support/Error.cpp
>>> index 7436a1fd38ee..151fcdcbfb27 100644
>>> --- a/llvm/lib/Support/Error.cpp
>>> +++ b/llvm/lib/Support/Error.cpp
>>> @@ -112,3 +112,17 @@ void report_fatal_error(Error Err, bool GenCrashDiag) {
>>> }
>>> 
>>> }
>>> +
>>> +
>>> +bool llvm::setABIBreakingChecks(bool Enabled) {
>>> +  static char abi_breaking_checks_enabled = 0;
>>> +  static char abi_breaking_checks_disabled = 0;
>>> +  if (Enabled)
>>> +abi_breaking_checks_enabled = 1;
>>> +  else
>>> +abi_breaking_checks_disabled = 1;
>>> +  if (abi_breaking_checks_enabled && abi_breaking_checks_disabled)
>>>

[lldb-dev] Bug in CommandObjectFrameVariable::DoExecute()

[Zachary Turner via lldb-dev]

On line 708 of CommandObjectFrame.cpp, I'm looking at this code:

options.SetRootValueObjectName(name_cstr);

This code occurs inside the else block of an if/else.  name_cstr is
declared prior to the if/else block and initialized to nullptr, but is only
ever set to something other than nullptr in the if branch.

So, this code is equivalent to

options.SetRootValueObjectName(nullptr);

Is this intended, and if not how would this bug manifest itself in terms of
incorrect behavior?
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

[lldb-dev] Refactoring in LLDB Windows Plugin

[Adrian McCarthy via lldb-dev]

If you're not working in LLDB's Windows process plugin, you probably don't
care about this message.

FYI:  I'm doing some refactoring (actually unfactoring) in the Windows
process plugin.  It'll take a series patches over a few days next week.  If
you're planning on working in this area, please let me know so we can
coordinate.

Details:

Last year, I factored the classes like ProcessWindows into pairs of classes
with names like ProcessWindows and ProcessWindowsLive so that I could
introduce classes like ProcessWindowsMiniDump that shared common code.  Now
that the Windows-specific minidump plugin has been superseded by the
cross-platform minidump plugin, this factoring is no longer necessary.
Since the factoring creates extra layers that make the code harder to
understand and maintain, I'd like to undo the split.

My plan is to do this in three NFC patches:

Patch 1.  Roll the functionality from the common classes into the -Live
classes.  This will eliminate everything under
Plugins/Process/Windows/Common and leave the functional code in
Process/Plugins/Windows/Live.

Patch 2.  Rename the -Live classes to shorter, more tractable names.

Patch 3.  Move the code up from the Live subdirectory so that it once again
lives in Plugins/Process/Windows.

Patches 2 and 3 could be done in a single step, but I think the history
will be easier to follow if I keep them separate.

If you have any concerns about this plan, please let me know.

Adrian.
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

Re: [lldb-dev] Bug in CommandObjectFrameVariable::DoExecute()

[Enrico Granata via lldb-dev]

On 11/18/16 3:55 PM, Zachary Turner via lldb-dev wrote:

On line 708 of CommandObjectFrame.cpp, I'm looking at this code:

options.SetRootValueObjectName(name_cstr);

This code occurs inside the else block of an if/else.  name_cstr is 
declared prior to the if/else block and initialized to nullptr, but is 
only ever set to something other than nullptr in the if branch.


So, this code is equivalent to

options.SetRootValueObjectName(nullptr);

Is this intended,

Probably not
and if not how would this bug manifest itself in terms of incorrect 
behavior?
That I am not sure about.. I tried a couple obvious things, but I 
couldn't make them fail. It's possible that some arcane combination of 
options would do it. OTOH, my hunch is that we could simply fix the bug. 
Trying a patch of the form


Index: source/Commands/CommandObjectFrame.cpp
===
--- source/Commands/CommandObjectFrame.cpp(revision 287375)
+++ source/Commands/CommandObjectFrame.cpp(working copy)
@@ -704,7 +704,7 @@
   options.SetFormat(format);
   options.SetVariableFormatDisplayLanguage(
   valobj_sp->GetPreferredDisplayLanguage());
-  options.SetRootValueObjectName(name_cstr);
+  options.SetRootValueObjectName(var_sp ? 
var_sp->GetName().AsCString() : nullptr);

   valobj_sp->Dump(result.GetOutputStream(), options);
 }
   }

is the first thing I would do. On the face of it, it looks correct 
behavior (the "root name" is the name of the variable being displayed). 
Still worth a test suite run of course - but if you run into any 
regressions due to the change, it's worth bringing them up. It's 
possible the test is testing the wrong behavior and we found our smoking 
gun in terms of actual incorrect behavior.



___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev


___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

Re: [lldb-dev] Bug in OptionValuePathMappings::SetValueFromString

[Zachary Turner via lldb-dev]

BTW, this exact same code seems to have been copied either to or from
OptionValueFileSpecList::SetValueFromString, as it appears to have the same
bug.  Again, not sure how to see it fail in the debugger, but maybe someone
knows?

On Fri, Nov 18, 2016 at 1:07 PM Zachary Turner  wrote:

> Also, I just noticed the loop only runs one time, so it appears equivalent
> to
>
> m_path_mappings.Remove(remove_indexes.size() - 1, m_notify_changes);
>
> which seems completely wrong
>
> On Fri, Nov 18, 2016 at 1:05 PM Zachary Turner  wrote:
>
> In the switch / case handler for eVarSetOperationRemove, there is the
> following code:
>
>   if (num_remove_indexes) {
> // Sort and then erase in reverse so indexes are always valid
> std::sort(remove_indexes.begin(), remove_indexes.end());
> for (size_t j = num_remove_indexes - 1; j < num_remove_indexes;
> ++j) {
>   m_path_mappings.Remove(j, m_notify_changes);
> }
>   }
>
> Should the line that calls Remove() not be like this:
>
>   m_path_mappings.Remove(*remove_indexes[j]*, m_notify_changes);
>
> ?  What effect will this have on LLDB?
>
>
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

Re: [lldb-dev] [llvm-dev] [RFC] Runtime checks for ABI breaking build of LLVM

[Mehdi Amini via lldb-dev]

> On Nov 18, 2016, at 3:45 PM, Mehdi Amini  wrote:
> 
> Hi,
> 
> I had to revert it, because it breaks building LLDB on MacOS.
> 
> It turns out it would break any client that is including llvm-config.h but 
> not linking to libLLVMSupport.
> So either:
> 
> - we shouldn’t allow to include llvm-config.h without linking to LLVM, in 
> which case I need to look a bit closer as of why lldb includes this header in 
> a context where they don’t link LLVM.
> - we should allow to include llvm-config.h without linking to LLVM (at least 
> libSupport). In which case we need a new solution for this feature: it can be 
> to use another header dedicated to this flag, that would be included only 
> from headers that contain the ABI break.

The second approach is implemented here:  https://reviews.llvm.org/D26876

I extracted the LLVM_ENABLE_ABI_BREAKING_CHECKS to its own header to have a 
fine granularity on which client needs to include the check.

— 
Mehdi




> 
> 
>> On Nov 16, 2016, at 12:12 PM, Mehdi Amini via llvm-dev 
>> mailto:llvm-...@lists.llvm.org>> wrote:
>> 
>>> 
>>> On Nov 16, 2016, at 12:05 PM, Jonathan Roelofs >> > wrote:
>>> 
>>> 
>>> 
>>> On 11/16/16 11:48 AM, Mehdi Amini via llvm-dev wrote:
 Hi all,
 
 An issue that come up from time to time and has cost hours for debug for
 many of us and users of LLVM is that an assert build isn’t ABI
 compatible with a release build.
 
 The CMake flags that controls this behavior is LLVM_ABI_BREAKING_CHECKS (
 
 *LLVM_ABI_BREAKING_CHECKS*:STRING
Used to decide if LLVM should be built with ABI breaking checks or
not. Allowed values
are WITH_ASSERTS (default), FORCE_ON and FORCE_OFF.  WITH_ASSERTS turns
on ABI breaking checks in an assertion enabled
build.  FORCE_ON (FORCE_OFF) turns them on (off) irrespective of
whether normal (NDEBUG-based) assertions are enabled or not. A
version of LLVM built with ABI breaking checks is not ABI compatible
with a version built without it.
 
 
 I propose to add a runtime check to detect when we have incorrectly
 setup build.
 
 The idea is that every translation unit that includes such header will
 get a weak definition of a symbol with an initializer calling the
 runtime check. The symbol name would be different if the ABI break is
 enabled or not.
>>> 
>>> Can it be made into a link-time check instead? I'm imagining something like:
>> 
>> I’d love to, but didn’t find a universal solution unfortunately :(
>> 
>>>  #if LLVM_ENABLE_ABI_BREAKING_CHECKS
>>>  extern int EnableABIBreakingChecks;
>>>  __attribute__((weak)) int *VerifyEnableABIBreakingChecks = 
>>> &EnableABIBreakingChecks;
>>>  #else
>>>  extern int DisableABIBreakingChecks;
>>>  __attribute__((weak)) int *VerifyDisableABIBreakingChecks = 
>>> &VDisableABIBreakingChecks;
>>>  #endif
>>> 
>>> in llvm-config.h.cmake, and:
>>> 
>>>  #if LLVM_ENABLE_ABI_BREAKING_CHECKS
>>>  int EnableABIBreakingChecks;
>>>  #else
>>>  int DisableABIBreakingChecks;
>>>  #endif
>>> 
>>> in Error.cpp.
>>> 
>>> Then it'll only link if Error.cpp's TU's setting of 
>>> LLVM_ENABLE_ABI_BREAKING_CHECKS matches that of the TU that includes 
>>> llvm-config.h
>> 
>> It seems that this work, I thought I tried exactly this but got lost on the 
>> whiteboard at some point!
>> 
>> Maybe because one drawback that I tried to avoid is that the export-list of 
>> a LLVM dylib would depend on the value of LLVM_ENABLE_ABI_BREAKING_CHECKS 
>> with this.
>> 
>> Thanks,
>> 
>> — 
>> Mehdi
>> 
>> 
>> 
>> 
>> 
>>> 
>>> 
 
 The runtime check maintains two boolean to track if it there is in the
 image at least a translation unit for each value of this flag. If both
 flags are set the process is aborted.
 
 The cost is *one* static initializer per DSO (or per image I believe).
 
 A straw-man patch (likely not windows compatible because of weak) is:
 
 diff --git a/llvm/include/llvm/Config/llvm-config.h.cmake
 b/llvm/include/llvm/Config/llvm-config.h.cmake
 index 4121e865ea00..4274c942d3b6 100644
 --- a/llvm/include/llvm/Config/llvm-config.h.cmake
 +++ b/llvm/include/llvm/Config/llvm-config.h.cmake
 @@ -80,4 +80,18 @@
 /* LLVM version string */
 #define LLVM_VERSION_STRING "${PACKAGE_VERSION}"
 
 +
 +#ifdef __cplusplus
 +namespace llvm {
 +bool setABIBreakingChecks(bool Enabled);
 +__attribute__((weak))
 +#if LLVM_ENABLE_ABI_BREAKING_CHECKS
 +bool
 +ABICheckEnabled = setABIBreakingChecks(true);
 +#else
 +bool ABICheckDisabled = setABIBreakingChecks(true);
>>> 
>>> Do you mean `false` here ^ ?
>>> 
 +#endif
 +}
 +#endif
 +
 #endif
 diff --git a/llvm/lib/Support/Error.cpp b/llvm/lib/Support/Error.cpp
 index 7436a1fd38ee..151fcdcbfb27 100644
 --- a/llvm/lib/Support/Error.cpp
 +++ b/llvm/lib/Support/Error.cpp