Cleanup duplicate static inline functions at/after linking by tautschnig · Pull Request #1987 · diffblue/cbmc

tautschnig · 2018-03-30T16:40:43Z

No description provided.

smowton · 2018-04-17T10:26:46Z

regression/ansi-c/linking1/other.c

+#ifndef TEST2
+#include "foo.h"
+#else
+inline void foo(int x)


Comment in either the code or the .desc files what you're expecting here. I might be being thick, but I don't follow what you're expecting to differ, except that the non-static inline foo would leave an external-linkage version of the function body around for a potential linked object expecting an extern void foo(int), which doesn't in fact exist in this test?

I have added explanations in the .desc files.

Aha, excellent, thankyou!

smowton · 2018-04-17T10:27:15Z

regression/ansi-c/static2/main2.c

 static int foo(int a)
 {
-  return a+1;
+  return a + 2;


This makes sure the functions are actually different so that the static variant doesn't get ignored after the changes in this PR.

smowton · 2018-04-17T10:32:10Z

src/goto-programs/link_goto_model.cpp

+      continue;
+
+    std::string new_name = id2string(rename_it->second);
+    std::string::size_type suffix_pos = new_name.find("$link");


Instead of this grungy string decomposition, how about annotating such symbols with a comment ID_C_original_function_identifier or such?

That would seem nice, but I'm struggling to find a good place to put it in a symbolt?

I've been sticking annotations like this on the type myself. We should agree on a policy though, or make a breaking change to add symbolt::attributes

I do think that the name and the type are really disjoint, so I'd much rather sort this out via some to-be-added symbolt::attributes.

smowton · 2018-04-17T10:33:10Z

src/linking/linking.cpp

-  return false;
+  return (new_symbol.is_file_local || old_symbol.is_file_local) &&
+         (new_symbol.type.id() != ID_code ||
+          new_symbol.value == exprt("compiled") ||


Factor out this magic tag exprt?

smowton · 2018-04-17T10:35:21Z

src/goto-programs/goto_program.cpp

  }
 }

+bool goto_programt::instructiont::equal(const instructiont &other) const


Nitpick: I'd expect equals, or operator==

smowton · 2018-04-17T10:35:41Z

src/goto-programs/goto_program.cpp


+bool goto_programt::instructiont::equal(const instructiont &other) const
+{
+  return type == other.type && code == other.code && guard == other.guard &&


Reading ease: suggest break after each &&

smowton · 2018-04-17T10:56:52Z

src/goto-programs/goto_program.cpp

+      return false;
+
+    long jump_difference1 = 0;
+    if(!ins.targets.empty())


Should compare other targets I think -- e.g. catch instructions have several targets.

tautschnig · 2018-04-19T08:09:52Z

@smowton: all comments either addressed or commented on. @peterschrammel If you have any tests exercising goto-diff beyond what's run as regular regression tests then it may be worth checking with those (due to my refactoring of equality tests).

smowton · 2018-04-19T09:19:33Z

Looks good, except I'd move the target distance comparison inside instructiont::equals and make goto_programt::equals trivial, as at the moment two different GOTOs would compare alike according to instructiont::equals but unequal in the context of a GOTO program, which doesn't seem quite right.

tautschnig · 2018-04-19T10:14:50Z

@smowton I was about to implement your suggestion but then figured I'd disagree when looking on how this would impact the unified diff implementation: consider if(x) goto L; some_code; more_code; L: a_target; vs. if(x) goto L; some_code; L: a_target; - I my opinion the programs are different, and the difference lies in some_code;, but all other instructions are the same. The fact that the relative jump distance differs is just an artefact of the additional instruction.

smowton · 2018-04-19T10:43:41Z

Hmmmm, okay, I won't argue the point further

peterschrammel · 2018-04-19T11:01:09Z

@tautschnig, we are retesting this version on our benchmarks.

peterschrammel · 2018-04-24T13:57:27Z

@tautschnig, the output is identical to developon our tests.

peterschrammel

Please rebase.

allredj

Passed Diffblue compatibility checks (cbmc commit: fd9896f).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/86271881

allredj

Passed Diffblue compatibility checks (cbmc commit: 623340a).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/88760998

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: 9a0de06).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/95548556

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: dcf5904).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/103507544

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: 15dbc4d).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/107061824

codecov · 2021-05-07T14:00:20Z

Codecov Report

Base: 78.43% // Head: 78.49% // Increases project coverage by +0.06% 🎉

Coverage data is based on head (1ee95db) compared to base (d7099ae).
Patch coverage: 58.62% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1987      +/-   ##
===========================================
+ Coverage    78.43%   78.49%   +0.06%     
===========================================
  Files         1670     1670              
  Lines       191671   191691      +20     
===========================================
+ Hits        150331   150470     +139     
+ Misses       41340    41221     -119

Impacted Files	Coverage Δ
src/goto-programs/link_goto_model.cpp	`78.00% <40.00%> (-9.50%)`	⬇️
src/linking/linking.cpp	`69.32% <100.00%> (ø)`
src/util/bitvector_expr.h	`97.41% <0.00%> (+0.46%)`	⬆️
src/util/symbol_table.cpp	`91.30% <0.00%> (+2.17%)`	⬆️
src/util/type.h	`98.68% <0.00%> (+7.89%)`	⬆️
src/util/validate_helpers.h	`100.00% <0.00%> (+11.11%)`	⬆️
src/big-int/bigint.cc	`89.00% <0.00%> (+14.26%)`	⬆️
src/util/validate_expressions.cpp	`93.75% <0.00%> (+15.62%)`	⬆️
src/util/string_hash.cpp	`45.45% <0.00%> (+18.18%)`	⬆️
src/util/validate_types.cpp	`93.33% <0.00%> (+56.66%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TGWDB · 2021-09-13T15:28:16Z

@tautschnig Any chance of a push to rerun the CI jobs (including the new/missing one)?

This will avoid some occurrences of function$linkN names that are confusing to users, in particular when specifying loop unwinding limits.

TGWDB · 2023-05-03T15:20:00Z

Closing due to age (no further comment on PR content), please reopen with rebase on develop if you intent to continue this work.

tautschnig requested review from kroening, peterschrammel and smowton as code owners March 30, 2018 16:40

tautschnig force-pushed the linking-cleanup branch from dd3d23d to 128d1ae Compare April 4, 2018 18:51

tautschnig assigned kroening Apr 4, 2018

tautschnig assigned tautschnig and unassigned kroening Apr 15, 2018

tautschnig force-pushed the linking-cleanup branch 2 times, most recently from 5fe23d7 to 914fcee Compare April 16, 2018 12:58

tautschnig requested review from forejtv and thk123 as code owners April 16, 2018 12:58

tautschnig force-pushed the linking-cleanup branch from 914fcee to 833f4cd Compare April 16, 2018 17:44

tautschnig assigned smowton, kroening and peterschrammel and unassigned tautschnig Apr 16, 2018

smowton reviewed Apr 17, 2018

View reviewed changes

tautschnig force-pushed the linking-cleanup branch from 833f4cd to 1bf46ee Compare April 19, 2018 08:07

tautschnig force-pushed the linking-cleanup branch from 1bf46ee to 3d199b1 Compare April 19, 2018 10:17

smowton approved these changes Apr 19, 2018

View reviewed changes

tautschnig force-pushed the linking-cleanup branch from 3d199b1 to d968793 Compare April 19, 2018 10:45

tautschnig unassigned smowton and kroening Apr 19, 2018

peterschrammel approved these changes Apr 24, 2018

View reviewed changes

tautschnig force-pushed the linking-cleanup branch from d16a5b3 to fd9896f Compare September 28, 2018 09:47

allredj reviewed Sep 28, 2018

View reviewed changes

tautschnig force-pushed the linking-cleanup branch from fd9896f to 623340a Compare October 22, 2018 15:46

allredj reviewed Oct 22, 2018

View reviewed changes

tautschnig force-pushed the linking-cleanup branch from 623340a to 9a0de06 Compare December 21, 2018 15:21

allredj reviewed Dec 21, 2018

View reviewed changes

tautschnig force-pushed the linking-cleanup branch from 9a0de06 to dcf5904 Compare March 7, 2019 08:49

allredj reviewed Mar 7, 2019

View reviewed changes

tautschnig force-pushed the linking-cleanup branch 2 times, most recently from 3b94bf0 to 15dbc4d Compare April 4, 2019 10:08

allredj reviewed Apr 4, 2019

View reviewed changes

tautschnig force-pushed the linking-cleanup branch from 15dbc4d to 2244100 Compare May 7, 2021 06:35

tautschnig requested a review from chrisr-diffblue as a code owner May 7, 2021 06:35

tautschnig force-pushed the linking-cleanup branch from 2244100 to 77bc8f1 Compare May 7, 2021 12:29

tautschnig force-pushed the linking-cleanup branch from 77bc8f1 to d34b1a9 Compare October 27, 2022 10:18

tautschnig requested a review from chris-ryder as a code owner October 27, 2022 10:18

tautschnig requested review from nwetzler and removed request for chrisr-diffblue, forejtv and thk123 November 4, 2022 22:15

tautschnig assigned nwetzler Nov 4, 2022

tautschnig force-pushed the linking-cleanup branch from d34b1a9 to 5a9e873 Compare November 20, 2022 21:46

TGWDB approved these changes Nov 23, 2022

View reviewed changes

tautschnig unassigned nwetzler Dec 16, 2022

Linking can safely clean up duplicates of static inline functions

1ee95db

This will avoid some occurrences of function$linkN names that are confusing to users, in particular when specifying loop unwinding limits.

tautschnig force-pushed the linking-cleanup branch from 5a9e873 to 1ee95db Compare February 23, 2023 10:06

TGWDB closed this May 3, 2023

Conversation

tautschnig commented Mar 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tautschnig commented Apr 19, 2018

Uh oh!

smowton commented Apr 19, 2018

Uh oh!

tautschnig commented Apr 19, 2018

Uh oh!

smowton commented Apr 19, 2018

Uh oh!

peterschrammel commented Apr 19, 2018

Uh oh!

peterschrammel commented Apr 24, 2018

Uh oh!

peterschrammel left a comment

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

TGWDB commented Sep 13, 2021

Uh oh!

TGWDB commented May 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov bot commented May 7, 2021 •

edited

Loading